Repository navigation
Support ADQL-2.1 OFFSET clause (Postgres and Oracle) - #268
Merged
Merged
Conversation
AdqlQuery rejected any query with a bare OFFSET clause as an "invalid ADQL keyword: LIMIT", since a Limit AST node with no LIMIT keyword looked identical to one with rowCount defaulted to 0. Fixing just that check would have been incomplete: QuerySelectDeParser separately used rowCount == 0 as a proxy for an explicit "TOP 0", which would have collapsed an offset-only query to "LIMIT 0" -- silently returning zero rows instead of skipping the requested number. Add an explicit rowCountSet flag to Limit so "no row cap was ever given" is distinguishable from "TOP 0 was given", and use it in both places. Also give OracleQuerySelectDeParser its own deparseLimit override, since Oracle needs ANSI "OFFSET n ROWS" rather than the bare "OFFSET n" Postgres/MySQL accept. Verified against a real PostgreSQL instance: TOP, bare OFFSET, TOP+OFFSET combined, and OFFSET past the end of the result set all return the correct rows, while a literal LIMIT keyword is still rejected. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…oracle Each module in this repo is released independently and consumed by downstream modules via a Maven version-range coordinate, not a local project reference. CI installs each module to the runner's local Maven repo in sequence, but when a changed module keeps its old version number, Gradle's range resolution treats that version as available in both mavenCentral() and mavenLocal() and prefers mavenCentral() (listed first), silently ignoring the freshly-built local artifact. That's what broke the previous commit's CI run: cadc-adql compiled against the old, unpatched cadc-jsqlparser-compat:0.6.6 from Maven Central instead of the locally-installed one with isRowCountSet(). Bumping the version of every module whose code changed here removes the ambiguity, verified against a clean-cache simulation of the CI sequence. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
Author
|
I have tried to correct the pipeline error (although I am not very familiar, and I am not sure how to trigger it again) (only allowed to be triggered by maintainers, I think). Please feel free to amend anything that is not aligned with your pipeline or code standards |
Member
|
I think pipeline has to run successfully once and then once I merge this then pipeline will trigger automatically in future. I think the versions bumps were the correct fix because mavenCentral is higher priority than mavenLocal. |
pdowler
approved these changes
Oct 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
AdqlQueryrejected any query containing a bareOFFSETclause asinvalid ADQL keyword: LIMIT, because aLimitAST node carrying only an offset was indistinguishable from one carrying a realLIMIT/TOP-derived row count (both defaultedrowCountto0).QuerySelectDeParserseparately treatedrowCount == 0as a signal for an explicitTOP 0, which would have silently collapsed an offset-only query intoLIMIT 0— returning zero rows instead of skipping the requested number, with no error raised.rowCountSetflag toLimit(cadc-jsqlparser-compat) so "no row cap was ever established" is distinguishable from "a row cap of exactly 0 was established", and used it in both the validator and the deparser.OracleQuerySelectDeParserinherited the fixed basedeparseLimit, which would have emitted the bareOFFSET nform Postgres/MySQL accept — invalid for Oracle, which requires the ANSIOFFSET n ROWSform. Gave it its own override.Test plan
cadc-adqlunit tests: bareOFFSET,OFFSETwithoutORDER BY,TOP n ... OFFSET mcombined, and the MySQL-styleLIMIT offset,row_countform still correctly rejected (uses theLIMITkeyword).cadc-tap-server-oracleunit tests: offset-only emitsOFFSET n ROWS;TOP n ... OFFSET mcombined still correctly wraps in theROWNUMsubselect with the offset preserved inside it.cadc-adqlandcadc-tap-server-oraclepass (the 7 pre-existing failures inTapSchemaReadAccessConverterTestreproduce identically against unmodifiedmain, confirmed by running the same suite without this change).TOP, bareOFFSET,TOP+OFFSETcombined, andOFFSETpast the end of the result set were executed directly, and all returned the correct rows.🤖 Generated with Claude Code