[FLINK-26520][table] Implement SEARCH operator in codegen #19001

slinkydeveloper · 2022-03-07T16:12:23Z

What is the purpose of the change

This PR implements the SEARCH operator in the codegen, and removes the scalar implementation of IN and NOT_IN. Now every scalar IN/NOT_IN using a constant set is implemented through SEARCH (following Calcite's development on the topic https://issues.apache.org/jira/browse/CALCITE-4173) and plans will only have SEARCH

Brief change log

Remove NOT_IN from the plans, which was added only by the ConvertToNotInOrInRule. With this commit every scalar IN/NOT_IN using a constant set is converted to SEARCH, otherwise it's converted to a chain of disjunctions (see RexUtil#expandSearch)
Now GenerateUtils#generateLiteral understands the Flink's internal type system. The conversion from Calcite's Comparable type system to Flink's internal type system is provided by a new function RexLiteralUtil#toFlinkInternalValue
Add SearchOperatorGen to implement the SEARCH operator starting from the previous generateIn function in ScalarOperatorGens.

Verifying this change

Existing code is already testing thoroughly IN/NOT_IN. I added an additional test for the plan

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): no
The public API, i.e., is any changed class annotated with @Public(Evolving): no

Documentation

Does this pull request introduce a new feature? no
If yes, how is the feature documented? not applicable

… as suggested by Calcite Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

flinkbot · 2022-03-07T16:18:38Z

CI report:

8cfccaa Azure: SUCCESS

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run azure re-run the last Azure build

…rnal type system. The conversion from Calcite's Comparable type system to Flink's internal type system is provided by a new function RexLiteralUtil#toFlinkInternalValue Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

slinkydeveloper · 2022-03-10T16:25:43Z

@flinkbot run azure

twalthr

Very nice refactoring @slinkydeveloper. I couldn't add much here. I hope the change is already covered by existing tests.

...link-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/GenerateUtils.scala

twalthr · 2022-03-14T14:02:55Z

...flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/CodeGenUtils.scala

    case td: TimestampData =>
      s"$TIMESTAMP_DATA.fromEpochMillis(${td.getMillisecond}L, ${td.getNanoOfMillisecond})"
+    case decimalData: DecimalData =>
+      s"""$DECIMAL_UTIL.castFrom(


nit: wouldn't unscaled long/bytes be cheaper? at least we could use the compact representation for simple numbers.

In this particular case, it doesn't really make a difference, as we store literal values in static variables anyway. I rather prefer to keep the implementation simple here.

twalthr · 2022-03-14T14:07:50Z

...-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/calls/LikeCallGen.scala


            if (noneMatcher.matches()) {
-              val reusePattern = ctx.addReusableStringConstants(newPattern)
+              val reusePattern = ctx.addReusableEscapedStringConstant(newPattern)


nit: method is quite long addReusableEscapedStringConstant -> addReusableEscapedString

The fact that is adding a constant is quite important, i wouldn't remove it from the name.

...-table-planner/src/main/scala/org/apache/flink/table/planner/plan/utils/RexLiteralUtil.scala

twalthr · 2022-03-14T14:09:59Z

...-table-planner/src/main/scala/org/apache/flink/table/planner/plan/utils/RexLiteralUtil.scala

+        toFlinkInternalValue(value, valueType.asInstanceOf[DistinctType].getSourceType)
+
+      case SYMBOL =>
+        value.asInstanceOf[Enum[_]]


So Calcite symbols remain? Maybe this should be mentioned in the JavaDocs.

Yep to make the result of this function compatible with generateLiterals

...ble-planner/src/main/scala/org/apache/flink/table/planner/codegen/CodeGeneratorContext.scala

twalthr · 2022-03-14T14:16:24Z

...-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/ExprCodeGenerator.scala

@@ -464,19 +465,10 @@ class ExprCodeGenerator(ctx: CodeGeneratorContext, nullableInput: Boolean)
  override def visitCall(call: RexCall): GeneratedExpression = {
    val resultType = FlinkTypeFactory.toLogicalType(call.getType)
    if (call.getKind == SqlKind.SEARCH) {


do we still need this early check here or can we simply use the regular switch/case list below?

We need this check because the generateCallExpression method accepts the operands already converted to expressions. But in case of search, the codegen uses Sarg directly.

...-planner/src/main/scala/org/apache/flink/table/planner/codegen/calls/SearchOperatorGen.scala

Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

twalthr

Thanks for the update @slinkydeveloper.

Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

Implements the SEARCH operator in the codegen and removes the scalar implementation of IN and NOT_IN. Now every scalar IN/NOT_IN using a constant set is implemented through SEARCH (following Calcite's development on the topic CALCITE-4173) and plans will only have SEARCH. This closes #19001.

Implements the SEARCH operator in the codegen and removes the scalar implementation of IN and NOT_IN. Now every scalar IN/NOT_IN using a constant set is implemented through SEARCH (following Calcite's development on the topic CALCITE-4173) and plans will only have SEARCH. This closes apache#19001.

[FLINK-26520][table] Remove NOT_IN from the plan, and just use SEARCH…

8f2a482

… as suggested by Calcite Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

rmetzger added the component=TableSQL/Planner label Mar 7, 2022

slinkydeveloper force-pushed the remove-search branch 2 times, most recently from 4991fc6 to 8882fd5 Compare March 10, 2022 11:10

slinkydeveloper added 2 commits March 10, 2022 12:20

[FLINK-26520][table] Implement SEARCH operator in codegen

fde3eaa

Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

slinkydeveloper force-pushed the remove-search branch from 8882fd5 to fde3eaa Compare March 10, 2022 11:21

twalthr requested changes Mar 14, 2022

View reviewed changes

[fixup] Feedback

4507f88

Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

twalthr approved these changes Mar 15, 2022

View reviewed changes

[fixup] Fix checkstyle

8cfccaa

Signed-off-by: slinkydeveloper <francescoguard@gmail.com>

twalthr closed this in ae6c8cf Mar 16, 2022

slinkydeveloper deleted the remove-search branch March 16, 2022 09:44

[FLINK-26520][table] Implement SEARCH operator in codegen #19001

[FLINK-26520][table] Implement SEARCH operator in codegen #19001

Uh oh!

Conversation

slinkydeveloper commented Mar 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is the purpose of the change

Brief change log

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

Uh oh!

flinkbot commented Mar 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI report:

Uh oh!

slinkydeveloper commented Mar 10, 2022

Uh oh!

twalthr left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

twalthr left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

slinkydeveloper commented Mar 7, 2022 •

edited

Loading

flinkbot commented Mar 7, 2022 •

edited

Loading