Repository navigation
Conversation
opensearch-project#5766 encoded the bounds into every table source in AstBuilder, but Relation joins its sources with a comma -- the same character the encoded block uses to separate its own three fields -- and TimeBounds.decode only peels the trailing block. A comma-separated source therefore left every earlier block behind in the index expression, which then split into bogus index names (and, for a cross-cluster source, read a colon inside a timestamp as a cluster prefix). opensearch-project#5852 fixed that by encoding only the last source, so the joined name ends in exactly one block. That is correct, but it makes AstBuilder depend on how Relation serialises its names three layers later, with a comment at the call site as the only link. Move the bounds onto Relation as a nullable field and append the block once inside getTableQualifiedName(), after the join. The method that joins is now the method that appends, so the constraint is local instead of implied across files. AstBuilder.relation() collapses to one line and withTimeBounds() is deleted. No behaviour change: the encoded name is byte-identical, the storage engine decodes it the same way, and "outermost source= only" is still decided by isMainSource. Relation keeps its existing Relation(List) and Relation(UnresolvedExpression) constructors, so all call sites and both subclasses are untouched. Its @tostring and @EqualsAndHashCode now include the new field; equality is unaffected when bounds are null, which is the only case AstDSL and the SQL parser produce. AstTimeBoundsTest matches the new rendering, since the bounds no longer appear inside tableNames. opensearch-project#5852's comma-separated case is kept as-is -- it asserts through getTableQualifiedName(), which is the right level -- and the plan walker gained a null guard, as Node.getChild() returns null by default. Verified: spotlessCheck and compileJava/compileTestJava across all modules; core, ppl, opensearch, sql and protocol unit tests; and CalciteTimeBoundsPruningIT at 12 tests in both pushdown configurations, including opensearch-project#5852's two comma-separated cases. Signed-off-by: Peng Huo <penghuo@gmail.com>
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
1 participant
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.
Why
opensearch-project#5766 carries the request's time bounds encoded into the table name. opensearch-project#5852 fixed a bug there: encoding every source produced a name
decodecould not read back, becauseRelationjoins sources with,— the same character the encoded block uses internally.opensearch-project#5852's fix encodes only the last source, so the joined name ends in exactly one block. It works, but it leaves
AstBuildercorrect only because of howRelation.getTableQualifiedName()joins names three layers later. Nothing links the two except a comment at the call site. Change that join and the bug returns — silently, since a malformed name fails at index resolution, far from the code that built it.Change
Move the bounds onto
Relationas a nullable field and append the block once insidegetTableQualifiedName(), after the join. The method that joins is now the method that appends, so the constraint holds locally instead of across files.relation()collapses to one line andwithTimeBoundsis deleted (−21/+1). No behaviour change: the encoded name is byte-identical, the storage engine decodes it the same way, and "outermostsource=only" is still decided byisMainSource.Review notes
Relation(List)/Relation(UnresolvedExpression)constructors are kept, so all 21 call sites and both subclasses are untouched.@ToString/@EqualsAndHashCodenow include the new field. Equality is unaffected when bounds are null (the only caseAstDSLand the SQL parser produce);toString()does change for bounded relations, hence the test regex change.TimeBoundsstill uses,unescaped, so a bound containing a comma silently disables pruning.Testing
spotlessCheckand compile across all modules; unit tests forcore/ppl/opensearch/sql/protocol;CalciteTimeBoundsPruningIT12 tests × both pushdown configurations — all pass, including opensearch-project#5852's comma-separated cases.