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

saurabhd336 · 2022-04-18T10:44:28Z

Instructions:

The PR has to be tagged with at least one of the following labels (*):
1. feature
2. bugfix
3. performance
4. ui
5. backward-incompat
6. release-notes (**)
Remove these instructions before publishing the PR.

(*) Other labels to consider:

testing
dependencies
docker
kubernetes
observability
security
code-style
extension-point
refactor
cleanup

(**) Use release-notes label for scenarios like:

New configuration options
Deprecation of configurations
Signature changes to public methods/interfaces
New plugins added or old plugins removed

saurabhd336 · 2022-04-18T10:44:53Z

GH issue: #8488

cc: @npawar @KKcorps

codecov-commenter · 2022-04-18T11:17:19Z

Codecov Report

Merging #8563 (74cd5d1) into master (82f7aef) will increase coverage by 6.76%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##             master    #8563      +/-   ##
============================================
+ Coverage     64.00%   70.76%   +6.76%     
+ Complexity     4294     4289       -5     
============================================
  Files          1641     1686      +45     
  Lines         86307    88229    +1922     
  Branches      13142    13351     +209     
============================================
+ Hits          55237    62435    +7198     
+ Misses        27071    21422    -5649     
- Partials       3999     4372     +373

Flag	Coverage Δ
integration1	`27.31% <0.00%> (?)`
integration2	`25.80% <0.00%> (?)`
unittests1	`67.00% <0.00%> (-0.04%)`	⬇️
unittests2	`14.14% <0.00%> (+0.01%)`	⬆️

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

Impacted Files	Coverage Δ
...ot/common/function/scalar/ArithmeticFunctions.java	`61.53% <0.00%> (-3.68%)`	⬇️
...nt/local/startree/v2/store/StarTreeDataSource.java	`40.00% <0.00%> (-13.34%)`	⬇️
...ot/segment/local/startree/OffHeapStarTreeNode.java	`72.22% <0.00%> (-5.56%)`	⬇️
.../helix/core/minion/MinionInstancesCleanupTask.java	`77.27% <0.00%> (-4.55%)`	⬇️
...e/impl/forward/FixedByteMVMutableForwardIndex.java	`90.90% <0.00%> (-3.04%)`	⬇️
...ugin/inputformat/avro/KafkaAvroMessageDecoder.java	`0.00% <0.00%> (ø)`
...che/pinot/plugin/metrics/yammer/YammerMetered.java	`25.00% <0.00%> (ø)`
...plugin/segmentuploader/SegmentUploaderDefault.java	`87.09% <0.00%> (ø)`
...t/server/starter/ServerQueriesDisabledTracker.java	`86.36% <0.00%> (ø)`
...pinot/plugin/metrics/yammer/YammerJmxReporter.java	`100.00% <0.00%> (ø)`
... and 383 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 82f7aef...74cd5d1. Read the comment docs.

Jackie-Jiang · 2022-04-18T19:08:50Z

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

Currently we don't support having different parameter type for the same scalar function, so we might need to use the same workaround similar to other functions by adding the type into the function name

Jackie-Jiang · 2022-04-18T19:11:04Z

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

Suggest adding a separate class for all the logical functions. See FilterKind enum for more supported logical functions

richardstartin · 2022-04-19T13:30:39Z

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

this isn't a robust way to compare double values for equality, I suggest using an epsilon of 1e-7

Ack.
Will return Double.compare(a, b) == 0 be right? From the looks of it, it'll converts both values to 754 notation and compare the bits

No, that would produce odd results too. doubles should be compared for equality within a tolerance because they are an approximation of a number and the error in the approximation should be accounted for.

For example, consider this program, which accumulates 1% 100 times:

double d = 0.00; for (int i = 0; i < 100; i++) { d += 0.01; } System.out.println(d == 1D); System.out.println(Double.compare(d, 1D)); System.out.println(Math.abs(d - 1D) < 1e-15);

it prints:

false 1 true

because 1% isn't finitely expressible in base 2. However, the quantity is clearly an approximation of 1. In order to make this function robust to arithmetic performed by transform functions (prior to persistence or at query time) equality should be verified relative to a small tolerance (1e-7 would be best).

Understood. Thanks @richardstartin. Updated the pr

richardstartin · 2022-04-21T08:06:27Z

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

+    if (!_method.isVarArgs()) {
+      int numParameters = _parameterClasses.length;
+      Preconditions.checkArgument(arguments.length == numParameters,
+          "Wrong number of arguments for method: %s, expected: %s, actual: %s",
+          _method, numParameters, arguments.length);
+      for (int i = 0; i < numParameters; i++) {
+        // Skip conversion for null
+        Object argument = arguments[i];
+        if (argument == null) {
+          continue;
+        }
+        // Skip conversion if argument can be directly assigned
+        Class<?> parameterClass = _parameterClasses[i];
+        Class<?> argumentClass = argument.getClass();
+        if (parameterClass.isAssignableFrom(argumentClass)) {
+          continue;
+        }
+
+        PinotDataType parameterType = _parameterTypes[i];
+        PinotDataType argumentType = FunctionUtils.getArgumentType(argumentClass);
+        Preconditions.checkArgument(parameterType != null && argumentType != null,
+            "Cannot convert value from class: %s to class: %s", argumentClass, parameterClass);
+        arguments[i] = parameterType.convert(argument, argumentType);
      }
-      // Skip conversion if argument can be directly assigned
-      Class<?> parameterClass = _parameterClasses[i];
-      Class<?> argumentClass = argument.getClass();
-      if (parameterClass.isAssignableFrom(argumentClass)) {
-        continue;
+    } else {
+      Class<?> parameterBaseClass = _parameterClasses[0].getComponentType();
+      for (Object argument : arguments) {
+        if (argument == null) {
+          continue;
+        }
+        if (!parameterBaseClass.isAssignableFrom(argument.getClass())) {
+          // TODO add conversion if classes don't match
+          throw new IllegalArgumentException(
+              "Argument class" + argument.getClass() + "can't be converted to " + parameterBaseClass);
+        }


I would be tempted to decompose this into a method for each branch so it's a bit less of a wall of code to read

richardstartin · 2022-04-21T08:13:32Z

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

+      if (_method.isVarArgs()) {
+        Class<?> parameterBaseClass = _parameterClasses[0].getComponentType();
+        // Convert arguments to base class
+        Class<?> klass = Arrays.stream(_method.getParameterTypes()).findFirst().get().getComponentType();


Support for varargs will already impose overheads, but streaming the parameter types will be unacceptably slow. This operation should be performed once and cached in the FunctionInvoker. It's probably better to store a prototype array in a field too, which you can just clone at invocation time.

richardstartin · 2022-04-21T08:15:25Z

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

+    if (functionInfoMap != null && functionInfoMap.containsKey(numParameters)) {
+      return functionInfoMap.get(numParameters);
+    } else {
+      return VARARGS_FUNCTION_INFO_MAP.getOrDefault(functionName, null);


Map#get does the same thing

KKcorps · 2022-04-21T08:15:58Z

I think there should be support for operators as well in filters such as ==, >= etc. rather than only relying on functional equivalents. However, that might increase the scope of this PR significantly so we can take that up later.

richardstartin · 2022-04-21T08:20:57Z

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

+  public static boolean and(Boolean... args) {
+    for (Boolean arg : args) {
+      if (!arg) {
+        return false;
+      }
+    }
+    return true;
+  }


This function is to support logical conjugation of predicates, but is it the right approach? This requires all expressions to be evaluated before they are combined, and prevents early termination. Early termination is not just a performance concern, but one of flexibility and correctness - it should be possible to express predicates which aren't safe to evaluate if another predicate acts as a guard to prevent its evaluation.

+1 on this. Will rework this to allow early termination.

Have tried to model these functions differently (to be able to support early termination) here https://github.com/apache/pinot/pull/8582/files

saurabhd336 · 2022-04-25T07:27:31Z

@richardstartin @Jackie-Jiang @KKcorps @npawar This PR adds support of "and" and "or" conjugates via varargs function. As described in @richardstartin's review comment, this prohibits early termination and forces all conditions to be evaluated before and / or can be applied on their results.
I've tried to address that here #8582, modelling the conjugations a little differently. Suggest to review that PR instead. If that PR seems alright conceptually, I'll close this one and continue on that.

saurabhd336 changed the title ~~Add common comparison scalar functions for filterConfig during ingestion~~ Add common comparison scalar functions for filterConfig during ingestion (WIP) Apr 18, 2022

Jackie-Jiang reviewed Apr 18, 2022

View reviewed changes

npawar added the enhancement label Apr 18, 2022

richardstartin reviewed Apr 19, 2022

View reviewed changes

Saurabh Dubey and others added 2 commits April 20, 2022 15:27

Add filter config scalar functions

0597ad1

Add array sum

b4068aa

saurabhd336 force-pushed the scalarFunctionsInFilterConfig branch from 744c09f to 30b9eff Compare April 20, 2022 10:06

Double comparison fix

7190ee6

saurabhd336 force-pushed the scalarFunctionsInFilterConfig branch from 30b9eff to 7190ee6 Compare April 20, 2022 10:30

Saurabh Dubey added 3 commits April 20, 2022 17:41

Add varargs support for scalar function invoker

a6b6eea

Remove unused field

816b7fb

Formatting

51eab5d

richardstartin reviewed Apr 21, 2022

View reviewed changes

saurabhd336 closed this Apr 27, 2022

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

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

Uh oh!

Conversation

saurabhd336 commented Apr 18, 2022

Uh oh!

saurabhd336 commented Apr 18, 2022

Uh oh!

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

Uh oh!

Codecov Report

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!

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

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!

KKcorps commented Apr 21, 2022

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 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov-commenter commented Apr 18, 2022 •

edited

Loading

richardstartin Apr 20, 2022 •

edited

Loading