Support conjugates for scalar functions, add more scalar functions #8582

saurabhd336 · 2022-04-22T10:42:14Z

This PR adds support for being able to use complex scalar function expressions for fitlterConfig. In addition to common comparison functions, support for AND, OR and NOT has been added.
GH issue: #8488

How to use

With this change, filterConfig's filterFunction can now be simple SQL like expressions. This simplifies filtering of records especially with Groovy expressions being disabled by default.

"ingestionConfig": {
      "filterConfig": {
        "filterFunction": "svInt = 123 AND svDouble <= 200"
      }

saurabhd336 · 2022-04-25T09:37:49Z

GH issue: #8488

codecov-commenter · 2022-04-25T11:07:39Z

Codecov Report

Merging #8582 (58739e4) into master (7edad89) will decrease coverage by 1.67%.
The diff coverage is 39.62%.

@@             Coverage Diff              @@
##             master    #8582      +/-   ##
============================================
- Coverage     70.59%   68.92%   -1.68%     
- Complexity     4412     4413       +1     
============================================
  Files          1667     1669       +2     
  Lines         87314    87365      +51     
  Branches      13085    13098      +13     
============================================
- Hits          61643    60218    -1425     
- Misses        21411    22962    +1551     
+ Partials       4260     4185      -75

Flag	Coverage Δ
integration1	`?`
integration2	`25.83% <0.00%> (+0.02%)`	⬆️
unittests1	`66.40% <39.62%> (-0.02%)`	⬇️
unittests2	`14.35% <0.00%> (+0.03%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ot/common/function/scalar/ComparisonFunctions.java	`14.28% <14.28%> (ø)`
...gment/local/function/InbuiltFunctionEvaluator.java	`65.42% <43.18%> (-13.46%)`	⬇️
.../pinot/common/function/scalar/ObjectFunctions.java	`50.00% <50.00%> (ø)`
...pinot/minion/exception/TaskCancelledException.java	`0.00% <0.00%> (-100.00%)`	⬇️
...nverttorawindex/ConvertToRawIndexTaskExecutor.java	`0.00% <0.00%> (-100.00%)`	⬇️
...e/pinot/common/minion/MergeRollupTaskMetadata.java	`0.00% <0.00%> (-94.74%)`	⬇️
...plugin/segmentuploader/SegmentUploaderDefault.java	`0.00% <0.00%> (-87.10%)`	⬇️
.../transform/function/MapValueTransformFunction.java	`0.00% <0.00%> (-85.30%)`	⬇️
...ot/common/messages/RoutingTableRebuildMessage.java	`0.00% <0.00%> (-81.82%)`	⬇️
...verttorawindex/ConvertToRawIndexTaskGenerator.java	`5.45% <0.00%> (-80.00%)`	⬇️
... and 137 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7edad89...58739e4. Read the comment docs.

Jackie-Jiang

This approach is better than #8563
Could you please add some description to the PR? Don't put the template as the description as that doesn't help reviewers understand the purpose of the PR

Jackie-Jiang · 2022-04-26T20:33:35Z

pinot-common/src/main/java/org/apache/pinot/common/request/context/ExpressionContext.java

(minor) Revert this file

Jackie-Jiang · 2022-04-26T20:36:40Z

...nt-local/src/main/java/org/apache/pinot/segment/local/function/InbuiltFunctionEvaluator.java

Suggest adding 3 execution nodes: AndExecutionNode, OrExecutionNode, NotExecutionNode. IMO no need to add the concept of ConjugateFunction

Jackie-Jiang · 2022-04-26T20:39:30Z

pinot-common/src/main/java/org/apache/pinot/common/function/scalar/ComparisonFunctions.java

User will mostly likely use the function name within the FilterKind, and we should include that in the alias

Jackie-Jiang · 2022-04-26T20:41:48Z

...al/src/test/java/org/apache/pinot/segment/local/recordtransformer/RecordTransformerTest.java

Does svInt = 123 AND svInt < 123 work? Users usually don't use eq or lte explicitly.
Also let's put some filter that can potentially match record to test the behavior

Is was LTE. Hence it worked :)
Had added these to be able to test and / or. Made the tests a bit more extensive.
All the filters actually match the record. SKIP_RECORD_KEY is being checked for.

Jackie-Jiang · 2022-04-29T00:55:38Z

pinot-common/src/main/java/org/apache/pinot/common/function/scalar/ObjectFunctions.java

These 2 methods need to be annotated with nullableParameters = true

Jackie-Jiang · 2022-04-29T00:58:40Z

...nt-local/src/main/java/org/apache/pinot/segment/local/function/InbuiltFunctionEvaluator.java

(nit) Redundant parenthesis, same for others

Suggested change

Boolean res = (Boolean) (executableNode.execute(row));

Boolean res = (Boolean) executableNode.execute(row);

Jackie-Jiang · 2022-04-29T00:59:51Z

pinot-common/src/main/java/org/apache/pinot/common/function/scalar/ObjectFunctions.java

We don't need to annotate the names because it matches the method name in the canonical format

Jackie-Jiang · 2022-04-29T01:02:09Z

pinot-common/src/main/java/org/apache/pinot/common/function/scalar/ComparisonFunctions.java

I wonder if users will use gt instead of >, and IIRC gt is not standard SQL syntax. We may just make the method name match the filter kind in camel case (they will be canonicalized to the same name)

Jackie-Jiang · 2022-04-29T01:03:09Z

...al/src/test/java/org/apache/pinot/segment/local/recordtransformer/RecordTransformerTest.java

Let's change the expression to svInt = 123 AND svDouble <= 200. Users won't explicitly write query in this format

npawar · 2022-04-29T23:25:50Z

...nt-local/src/main/java/org/apache/pinot/segment/local/function/InbuiltFunctionEvaluator.java

need to make sure these work case-insensitive way (all other scalar functions do)

This will work in a case insensitive way due to https://github.com/apache/pinot/blob/master/pinot-common/src/main/java/org/apache/pinot/common/request/context/FunctionContext.java#L42

Jackie-Jiang

LGTM, only minor/nit comments

Jackie-Jiang · 2022-05-03T18:52:27Z

...nt-local/src/main/java/org/apache/pinot/segment/local/function/InbuiltFunctionEvaluator.java

(minor)

Suggested change

Preconditions.checkState(numArguments == 1, "Unsupported number of arguments for NOT operator");

Preconditions.checkState(numArguments == 1, "NOT function expects 1 argument, got: %s", numArguments);

Jackie-Jiang · 2022-05-03T18:54:32Z

pinot-common/src/main/java/org/apache/pinot/common/function/scalar/ComparisonFunctions.java

(nit)

Suggested change

return (Math.abs(a - b) >= DOUBLE_COMPARISON_TOLERANCE);

return Math.abs(a - b) >= DOUBLE_COMPARISON_TOLERANCE;

Jackie-Jiang · 2022-05-03T18:54:52Z

pinot-common/src/main/java/org/apache/pinot/common/function/scalar/ComparisonFunctions.java

(nit)

Suggested change

return (Math.abs(a - b) < DOUBLE_COMPARISON_TOLERANCE);

return Math.abs(a - b) < DOUBLE_COMPARISON_TOLERANCE;

Jackie-Jiang · 2022-05-03T18:55:28Z

pinot-common/src/main/java/org/apache/pinot/common/function/scalar/ComparisonFunctions.java

(nit)

Suggested change

return (val > a) && (val < b);

return val > a && val < b;

Jackie-Jiang · 2022-05-03T18:56:10Z

pinot-common/src/main/java/org/apache/pinot/common/function/scalar/ObjectFunctions.java

(minor)

Suggested change

public static boolean isNull(Object obj) {

public static boolean isNull(@Nullable Object obj) {

Jackie-Jiang · 2022-05-03T18:56:31Z

pinot-common/src/main/java/org/apache/pinot/common/function/scalar/ObjectFunctions.java

(minor)

Suggested change

public static boolean isNotNull(Object obj) {

public static boolean isNotNull(@Nullable Object obj) {

npawar · 2022-05-03T22:34:54Z

...al/src/test/java/org/apache/pinot/segment/local/recordtransformer/RecordTransformerTest.java

this shouldn't work rt? it should be svInt == 123

In SQL, we use = for comparison, e.g. SELECT * FROM myTable WHERE svInt = 123

saurabhd336 mentioned this pull request Apr 25, 2022

Add common comparison scalar functions for filterConfig during ingestion (WIP) #8563

Closed

saurabhd336 force-pushed the conjugationQueryOperators branch from be23f26 to 73d8d3d Compare April 25, 2022 09:17

saurabhd336 force-pushed the conjugationQueryOperators branch from eefad9f to 7a5fff1 Compare April 26, 2022 09:01

Jackie-Jiang reviewed Apr 26, 2022

View reviewed changes

saurabhd336 force-pushed the conjugationQueryOperators branch 2 times, most recently from 612bcd7 to e1c216e Compare April 28, 2022 13:05

Jackie-Jiang reviewed Apr 29, 2022

View reviewed changes

saurabhd336 force-pushed the conjugationQueryOperators branch 3 times, most recently from d48c94a to 70aae26 Compare April 29, 2022 12:34

saurabhd336 changed the title ~~Support conjugates for scalar functions (with early termination) (WIP)~~ Support conjugates for scalar functions, add more scalar functions Apr 29, 2022

npawar reviewed Apr 29, 2022

View reviewed changes

Jackie-Jiang approved these changes May 3, 2022

View reviewed changes

npawar reviewed May 3, 2022

View reviewed changes

Saurabh Dubey added 14 commits May 4, 2022 10:07

Add conjugation function type

22844e5

Lint

6d2c2e2

Fix tests

9f20103

Lint

ede82c5

Refactor

e2b907e

Add more functions

ee851e7

Lint

941d976

Fix tests

07c36bc

Get rid of switch case in online path

e374ef9

Review comments + more scalar functions

c875ffe

Lint fix

64ea5f6

Review comments

f35a36c

Add more tests

4f77c2e

Fix tests

5fb8d9a

xiangfu0 approved these changes May 4, 2022

View reviewed changes

saurabhd336 force-pushed the conjugationQueryOperators branch 2 times, most recently from ed6183a to 60d4227 Compare May 4, 2022 06:56

Review comments

58739e4

saurabhd336 force-pushed the conjugationQueryOperators branch from 60d4227 to 58739e4 Compare May 4, 2022 10:57

Jackie-Jiang added the enhancement label May 4, 2022

Jackie-Jiang merged commit d574085 into apache:master May 4, 2022

saurabhd336 mentioned this pull request Oct 19, 2022

Add docs for filter config and java client controller url init pinot-contrib/pinot-docs#131

Merged

	Boolean res = (Boolean) (executableNode.execute(row));
	Boolean res = (Boolean) executableNode.execute(row);

	Preconditions.checkState(numArguments == 1, "Unsupported number of arguments for NOT operator");
	Preconditions.checkState(numArguments == 1, "NOT function expects 1 argument, got: %s", numArguments);

	return (Math.abs(a - b) >= DOUBLE_COMPARISON_TOLERANCE);
	return Math.abs(a - b) >= DOUBLE_COMPARISON_TOLERANCE;

	return (Math.abs(a - b) < DOUBLE_COMPARISON_TOLERANCE);
	return Math.abs(a - b) < DOUBLE_COMPARISON_TOLERANCE;

	public static boolean isNull(Object obj) {
	public static boolean isNull(@Nullable Object obj) {

	public static boolean isNotNull(Object obj) {
	public static boolean isNotNull(@Nullable Object obj) {

Support conjugates for scalar functions, add more scalar functions #8582

Support conjugates for scalar functions, add more scalar functions #8582

Uh oh!

Conversation

saurabhd336 commented Apr 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How to use

Uh oh!

saurabhd336 commented Apr 25, 2022

Uh oh!

codecov-commenter commented Apr 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saurabhd336 commented Apr 22, 2022 •

edited

Loading

codecov-commenter commented Apr 25, 2022 •

edited

Loading