-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Add support for EXTRACT syntax and converts it to appropriate Pinot expression #9184
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
9e508a9 to
80123d6
Compare
1b53155 to
3f1fcd1
Compare
Jackie-Jiang
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for help contribute this!
EXTRACT can be modeled as a transform function in Pinot, and we want to change the CalciteSqlParser to support its syntax, and parse it to a function: extract(field, expression) where field is a literal.
We already have all kinds of time conversion functions available (see DateTimeTransformFunction), we may add a ExtractTransformFunction to support the functionality
|
thanks, @Jackie-Jiang for providing context. Very helpful. I am trying to understand how
|
|
To support this feature, we need 2 parts of the changes:
|
|
Hey @Jackie-Jiang , I have completed part 1 of the task. For Part2: |
f986bc6 to
0c381c6
Compare
0c381c6 to
cb9bb04
Compare
Jackie-Jiang
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TransformFunction is applied in the TransformOperator. You shouldn't need to change that though
pinot-clients/pinot-java-client/src/main/java/org/apache/pinot/client/PinotClientTransport.java
Outdated
Show resolved
Hide resolved
pinot-common/src/main/java/org/apache/pinot/common/request/Expression.java
Outdated
Show resolved
Hide resolved
pinot-common/src/main/java/org/apache/pinot/common/request/ExpressionType.java
Outdated
Show resolved
Hide resolved
pinot-common/src/main/java/org/apache/pinot/sql/FilterKind.java
Outdated
Show resolved
Hide resolved
pinot-common/src/main/java/org/apache/pinot/common/function/TransformFunctionType.java
Outdated
Show resolved
Hide resolved
pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
Outdated
Show resolved
Hide resolved
pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
Outdated
Show resolved
Hide resolved
2d0f715 to
2e02312
Compare
2e02312 to
9b3b27d
Compare
pinot-common/src/main/java/org/apache/pinot/common/request/Expression.java
Outdated
Show resolved
Hide resolved
|
PTAL @Jackie-Jiang when you get a chance. Thanks! |
...est/java/org/apache/pinot/core/operator/transform/function/ExtractTransformFunctionTest.java
Show resolved
Hide resolved
Jackie-Jiang
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly good
...rc/main/java/org/apache/pinot/core/operator/transform/function/ExtractTransformFunction.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/apache/pinot/core/operator/transform/function/ExtractTransformFunction.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/apache/pinot/core/operator/transform/function/ExtractTransformFunction.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/apache/pinot/core/operator/transform/function/ExtractTransformFunction.java
Outdated
Show resolved
Hide resolved
...est/java/org/apache/pinot/core/operator/transform/function/ExtractTransformFunctionTest.java
Show resolved
Hide resolved
6daadab to
40837a0
Compare
pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java
Outdated
Show resolved
Hide resolved
pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/apache/pinot/core/operator/transform/function/ExtractTransformFunction.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/apache/pinot/core/operator/transform/function/ExtractTransformFunction.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/apache/pinot/core/operator/transform/function/ExtractTransformFunction.java
Show resolved
Hide resolved
Jackie-Jiang
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with minor comments
pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java
Outdated
Show resolved
Hide resolved
...est/java/org/apache/pinot/core/operator/transform/function/ExtractTransformFunctionTest.java
Outdated
Show resolved
Hide resolved
...est/java/org/apache/pinot/core/operator/transform/function/ExtractTransformFunctionTest.java
Outdated
Show resolved
Hide resolved
32e9c28 to
cc713c7
Compare
Jackie-Jiang
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems there are other formatting issues. Can you please take a look at the linter failure?
...est/java/org/apache/pinot/core/operator/transform/function/ExtractTransformFunctionTest.java
Outdated
Show resolved
Hide resolved
siddharthteotia
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just briefly went over it since it has already been thoroughly reviewed. LGTM
Description
This PR adds support EXTRACT syntax and converts it to its Pinot expression.
This PR will solve the following issue -- #9075
Testing
Verified the desired behavior locally by running CalciteSqlCompilerTest