Extend the PARTITIONED BY clause to accept string literals for the time partitioning#15836
Merged
zachjsh merged 12 commits intoapache:masterfrom Feb 9, 2024
Merged
Conversation
abhishekrb19
reviewed
Feb 6, 2024
kgyrtkirk
reviewed
Feb 6, 2024
| | | ||
| e = Expression(ExprContext.ACCEPT_SUB_QUERY) | ||
| { | ||
| granularity = DruidSqlParserUtils.convertSqlNodeToGranularityThrowingParseExceptions(e); |
Member
There was a problem hiding this comment.
I think some validation should happen here; I don't see a nice way to do it but I've found this:
result = new SqlLiteral(DruidSqlParserUtils.convertSqlNodeToGranularityThrowingParseExceptions(e), SYMBOL, getPos());
which might work (but the typeName must be supplied - other candidate could be: UNKNOWN )
Contributor
Author
There was a problem hiding this comment.
Added validation here in another way, let me know if ok.
Member
There was a problem hiding this comment.
ok - but why not do the conversion here - I think you might also able to do similar thing on all the other branches
result = new SqlLiteral(DruidSqlParserUtils.convertSqlNodeToGranularityThrowingParseExceptions(e), SYMBOL, getPos());
you could shortcut the transient String + Symbol stuff
Contributor
Author
There was a problem hiding this comment.
Added a new SqlNode type, let me know if good now.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
abhishekrb19
reviewed
Feb 7, 2024
abhishekrb19
reviewed
Feb 8, 2024
Contributor
abhishekrb19
left a comment
abhishekrb19
left a comment
There was a problem hiding this comment.
Had a few questions and suggestions. Also, to future proof this new functionality, I would recommend adding tests in different parts of the code in case things get lost with refactoring or something breaks between Calcite upgrades. For example, https://github.com/apache/druid/blob/master/sql/src/test/java/org/apache/druid/sql/calcite/CalciteInsertDmlTest.java#L608 could be extended to test the newly added literal support.
abhishekrb19
approved these changes
Feb 9, 2024
Contributor
abhishekrb19
left a comment
abhishekrb19
left a comment
There was a problem hiding this comment.
Thanks for the changes @zachjsh!
kgyrtkirk
approved these changes
Feb 9, 2024
Comment on lines
+38
to
+39
| private String unparseString; | ||
| private Granularity granularity; |
| @Override | ||
| public void unparse(SqlWriter writer, int leftPrec, int rightPrec) | ||
| { | ||
| if (unparseString != null) { |
Member
There was a problem hiding this comment.
note: not sure if you need to be defensive here; its @NonNull in the constructor
| } | ||
| } | ||
|
|
||
| private static Granularity convertSqlIdentiferToGranularity(SqlIdentifier identifier) |
Member
There was a problem hiding this comment.
these 2 methods are almost identical; the difference is how they get to the String ; can you leave just one with a String arg - and do the X2String conversion before calling that method?
zachjsh
added a commit
to zachjsh/druid
that referenced
this pull request
Feb 24, 2024
Merged
10 tasks
zachjsh
added a commit
that referenced
this pull request
Mar 25, 2024
…urce input expressions (#15962) * * address remaining comments from #15836 * * address remaining comments from #15908 * * add test that exposes relational algebra issue * * simplify test exposing issue * * fix * * add tests for sealed / non-sealed * * update test descriptions * * fix test failure when -Ddruid.generic.useDefaultValueForNull=true * * check type assignment based on natice Druid types * * add tests that cover missing jacoco coverage * * add replace tests * * add more tests and comments about column ordering * * simplify tests * * review comments * * remove commented line * * STRING family types should be validated as non-null
abhishekrb19
changed the title
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
5 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.
Description
This PR contains a portion of the changes from the inactive draft PR for integrating the catalog with the Calcite planner #13686 from @paul-rogers, extending the PARTITION BY clause to accept string literals for the time partitioning
This PR has: