Add support for EXTRACT syntax and converts it to appropriate Pinot expression by tanmesh · Pull Request #9184 · apache/pinot

tanmesh · 2022-08-09T16:02:21Z

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

Jackie-Jiang

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

tanmesh · 2022-08-15T17:40:43Z

thanks, @Jackie-Jiang for providing context. Very helpful.

I am trying to understand how ExtractTransformFunction as you suggested will be used in the overall flow (write path and read path). Few follow-up ques from my investigation:

Do we store the transformed data (after applying transformations as defined here on raw data) into segments or only raw data into segments and apply the transformations on the fly during Pinot query execution?

Jackie-Jiang · 2022-08-15T17:49:44Z

ExtractTransformFunction will only be applied at query time. If we want to support ingestion transform, we need to add a ScalarFunction for the extract (you may take a look at DateTimeFunctions` class).

To support this feature, we need 2 parts of the changes:

Calcite parser change to support extract syntax and parse the query into extract(field, expression)
Add ExtractTransformFunction or ScalarFunction to support query/ingestion time transform

tanmesh · 2022-08-17T18:56:50Z

Hey @Jackie-Jiang , I have completed part 1 of the task.

For Part2:
Can you please help me point to where ‘DateTimeTransformFunction’ (or other query time transformers defined here) are getting applied? I was able to locate TransformFunctionFactory but couldn’t locate where transformations are actually getting applied.

Jackie-Jiang

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

pinot-common/src/thrift/query.thrift

pinot-common/src/main/java/org/apache/pinot/common/request/Expression.java

pinot-common/src/main/java/org/apache/pinot/common/request/ExpressionType.java

pinot-common/src/main/java/org/apache/pinot/sql/FilterKind.java

pinot-common/src/main/java/org/apache/pinot/common/function/TransformFunctionType.java

pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java

pinot-common/src/main/java/org/apache/pinot/common/request/Expression.java

tanmesh · 2022-08-19T18:59:11Z

PTAL @Jackie-Jiang when you get a chance. Thanks!

...est/java/org/apache/pinot/core/operator/transform/function/ExtractTransformFunctionTest.java

Jackie-Jiang

Mostly good

...rc/main/java/org/apache/pinot/core/operator/transform/function/ExtractTransformFunction.java

...est/java/org/apache/pinot/core/operator/transform/function/ExtractTransformFunctionTest.java

pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java

...rc/main/java/org/apache/pinot/core/operator/transform/function/ExtractTransformFunction.java

Jackie-Jiang

LGTM with minor comments

pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java

...est/java/org/apache/pinot/core/operator/transform/function/ExtractTransformFunctionTest.java

Jackie-Jiang

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

siddharthteotia

Just briefly went over it since it has already been thoroughly reviewed. LGTM

tanmesh force-pushed the tm/exact_syntax branch 2 times, most recently from 9e508a9 to 80123d6 Compare August 9, 2022 23:41

tanmesh changed the title ~~[WIP] working on the issue to support EXTRACT syntax~~ working on the issue to support EXTRACT syntax Aug 10, 2022

tanmesh changed the title ~~working on the issue to support EXTRACT syntax~~ Add support for EXTRACT syntax and converts it to appropriate Pinot expression Aug 10, 2022

mayankshriv requested a review from Jackie-Jiang August 10, 2022 03:08

Tanmesh added 2 commits August 10, 2022 07:38

supporting EXTRACT syntax

dec524c

fixed the licence header

3f1fcd1

tanmesh force-pushed the tm/exact_syntax branch from 1b53155 to 3f1fcd1 Compare August 10, 2022 14:39

Jackie-Jiang reviewed Aug 12, 2022

View reviewed changes

tanmesh changed the title ~~Add support for EXTRACT syntax and converts it to appropriate Pinot expression~~ [WIP] Add support for EXTRACT syntax and converts it to appropriate Pinot expression Aug 17, 2022

tanmesh force-pushed the tm/exact_syntax branch 2 times, most recently from f986bc6 to 0c381c6 Compare August 17, 2022 21:08

completed part1 -- updated CalciteSqlParser to support Extract query

cb9bb04

tanmesh force-pushed the tm/exact_syntax branch from 0c381c6 to cb9bb04 Compare August 17, 2022 21:19

Jackie-Jiang reviewed Aug 17, 2022

View reviewed changes

tanmesh force-pushed the tm/exact_syntax branch from 2d0f715 to 2e02312 Compare August 18, 2022 01:17

fixed based on the comments left

9b3b27d

tanmesh force-pushed the tm/exact_syntax branch from 2e02312 to 9b3b27d Compare August 18, 2022 01:19

Jackie-Jiang reviewed Aug 18, 2022

View reviewed changes

pinot-common/src/main/java/org/apache/pinot/common/request/Expression.java Outdated Show resolved Hide resolved

Tanmesh added 2 commits August 18, 2022 22:23

adding ExtractTransformFunction

f11a5b2

tmp

6062267

tanmesh changed the title ~~[WIP] Add support for EXTRACT syntax and converts it to appropriate Pinot expression~~ Add support for EXTRACT syntax and converts it to appropriate Pinot expression Aug 19, 2022

tanmesh commented Aug 19, 2022

View reviewed changes

...est/java/org/apache/pinot/core/operator/transform/function/ExtractTransformFunctionTest.java Show resolved Hide resolved

Jackie-Jiang reviewed Aug 22, 2022

View reviewed changes

adding changes in ExtractTransformFunction based on comments

40837a0

tanmesh force-pushed the tm/exact_syntax branch from 6daadab to 40837a0 Compare August 22, 2022 22:14

Jackie-Jiang reviewed Aug 23, 2022

View reviewed changes

added Field enum and updated based on comments

c6f5183

Jackie-Jiang approved these changes Aug 25, 2022

View reviewed changes

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

tanmesh commented Aug 25, 2022

View reviewed changes

...est/java/org/apache/pinot/core/operator/transform/function/ExtractTransformFunctionTest.java Outdated Show resolved Hide resolved

updating changes after reformatting

cc713c7

tanmesh force-pushed the tm/exact_syntax branch from 32e9c28 to cc713c7 Compare August 26, 2022 21:01

Jackie-Jiang reviewed Aug 27, 2022

View reviewed changes

...est/java/org/apache/pinot/core/operator/transform/function/ExtractTransformFunctionTest.java Outdated Show resolved Hide resolved

Tanmesh added 2 commits August 26, 2022 17:41

fixed some lint errors

b880950

fixing checkstyle errors

7f0ad5c

xiangfu0 approved these changes Sep 6, 2022

View reviewed changes

siddharthteotia approved these changes Sep 7, 2022

View reviewed changes

siddharthteotia merged commit 857d277 into apache:master Sep 7, 2022

siddharthteotia added the feature label Sep 7, 2022

Jackie-Jiang mentioned this pull request Jun 22, 2024

Support EXTRACT as scalar function #13462

Closed

This was referenced Jun 23, 2024

Support 'EXTRACT' as a scalar function #13463

Merged

Fix 'WEEK' transform function in multi-stage query engine #13483

Merged

Conversation

tanmesh commented Aug 9, 2022

Description

Testing

Uh oh!

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Uh oh!

tanmesh commented Aug 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jackie-Jiang commented Aug 15, 2022

Uh oh!

tanmesh commented Aug 17, 2022

Uh oh!

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tanmesh commented Aug 19, 2022

Uh oh!

Uh oh!

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

siddharthteotia left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tanmesh commented Aug 15, 2022 •

edited

Loading