AshharAhmadKhan commented on PR #6494:
URL: https://github.com/apache/fineract/pull/6494#issuecomment-6346581999
> Hi @AshharAhmadKhan, thanks for the PR. The bug is real, but this
regresses valid reports.
>
> * `SELECT a FROM t ORDER BY b` (or `ORDER BY id` with `l.id AS loan_id`)
runs fine today. After the lift the ORDER BY sits on `select x.* from (...) x`,
where `b` / `id` are not columns of `x`, so the report fails with "unknown
column". Reports are user-defined SQL in `stretchy_report`, so this can break
existing ones at runtime. Why is it safe to lift a clause that may not resolve?
Can we bail out unless every sort key is provably an output column?
> * The dot-qualified guard means the most common shape (`ORDER BY
l.account_no`) is not fixed at all, and it runs over string literals and
comments (`ORDER BY FIELD(status,'a.b')`).
> * The hand-rolled scanner is ~150 lines: backslash-escaped quotes
(`'it\'s'`), backtick/double-quoted identifiers, `E'..'` / `$$` strings and
nested comments are not handled, and everything after the last top-level ORDER
BY (`FOR UPDATE`, `LIMIT x, y`, a trailing `;`) is lifted, not just
LIMIT/OFFSET. Do we want to own a mini SQL lexer on every report path? Is there
a parser we already ship?
> * `wrapSQL` is called from 4 sites in `ReadReportingServiceImpl`, one
wrapping `inputSql` for parameter lookups. Does lifting change paging or count
queries there? Do we have evidence PostgreSQL is affected?
> * Tests: missing ORDER BY on a non-selected column, alias vs original
name, UNION with trailing ORDER BY, trailing semicolon, escaped quote, and
nothing runs against a real DB. `GenericDataServiceImplTest` duplicates a test
in `ReportSqlOrderByLifterTest`. Stray blank line / dangling comment in
`wrapSQL`.
> Hi @AshharAhmadKhan, thanks for the PR. The bug is real, but this
regresses valid reports.
>
> * `SELECT a FROM t ORDER BY b` (or `ORDER BY id` with `l.id AS loan_id`)
runs fine today. After the lift the ORDER BY sits on `select x.* from (...) x`,
where `b` / `id` are not columns of `x`, so the report fails with "unknown
column". Reports are user-defined SQL in `stretchy_report`, so this can break
existing ones at runtime. Why is it safe to lift a clause that may not resolve?
Can we bail out unless every sort key is provably an output column?
> * The dot-qualified guard means the most common shape (`ORDER BY
l.account_no`) is not fixed at all, and it runs over string literals and
comments (`ORDER BY FIELD(status,'a.b')`).
> * The hand-rolled scanner is ~150 lines: backslash-escaped quotes
(`'it\'s'`), backtick/double-quoted identifiers, `E'..'` / `$$` strings and
nested comments are not handled, and everything after the last top-level ORDER
BY (`FOR UPDATE`, `LIMIT x, y`, a trailing `;`) is lifted, not just
LIMIT/OFFSET. Do we want to own a mini SQL lexer on every report path? Is there
a parser we already ship?
> * `wrapSQL` is called from 4 sites in `ReadReportingServiceImpl`, one
wrapping `inputSql` for parameter lookups. Does lifting change paging or count
queries there? Do we have evidence PostgreSQL is affected?
> * Tests: missing ORDER BY on a non-selected column, alias vs original
name, UNION with trailing ORDER BY, trailing semicolon, escaped quote, and
nothing runs against a real DB. `GenericDataServiceImplTest` duplicates a test
in `ReportSqlOrderByLifterTest`. Stray blank line / dangling comment in
`wrapSQL`.
Thanks @galovics, you're right and I checked your points.
I ran the lifted ORDER BY and It fails on a non-selected column ("column
other does not exist"), on the original name when the select aliases it (`ORDER
BY id` with `l.id AS loan_id`), and on qualified names (`ORDER BY
l.account_no`, "missing FROM-clause entry"). So the lift can break reports that
work today.
I also scanned the 260 distinct report and parameter SQL texts in the
changelogs. About 151 have a top-level ORDER BY, and roughly 96 of those use
qualified names like `r.report_name` or `ounder.hierarchy`, which my guard
skips. So even if the lift was safe, it would only help a small part of them.
This was a rough regex scan, not a parser. I don't want to keep patching a
lexer.
Other things I found:
- `wrapSQL` has 3 callers in `ReadReportingServiceImpl`, not 4. Only the one
that wraps the user report SQL (line 211) matters for ordering. The other two
wrap fixed lookups with no ORDER BY. No count or paging query goes through it.
- On PostgreSQL I could not reproduce the problem. The wrapped query
returned the same order as the plain one, and the plan flattens the subquery.
The ticket does not say which DB is affected. It also says "numerical order"
while `account_no` is a varchar.
- The wrapper looks like it comes from 2012 ("used cached resultset for
reporting") and the comment about the Sun CachedRowSet bug.
`fillGenericResultSet` reads `getColumnName`, not `getColumnLabel`. On
PostgreSQL both return the alias, with or without the wrapper. I believe on
MySQL/MariaDB `getColumnName` can return the underlying column instead of the
alias, but I haven't verified that.
What I'd like to do: drop the lifter and remove the wrapper for the user
report SQL instead. I would read `getColumnLabel` in `fillGenericResultSet` so
alias names stay the same. That fixes ordering for every ORDER BY shape and
needs no SQL parsing. I would test it on MariaDB, MySQL and PostgreSQL before
updating the PR.
Two questions before I rework it:
1. Are you ok with removing the wrapper, or do you know other reasons it's
there?
2. Do you know which DB the reporter hit?
Hey @adamsaghy, I would like your input here too. You asked before whether
this wrapper is still needed, so I'd like to know if you are fine with this
direction.
If you'd rather keep the wrapper, I'll close this PR and not go further with
the lexer approach.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
