FrankChen021 commented on code in PR #20722:
URL: https://github.com/apache/druid/pull/20722#discussion_r4367521456
##########
sql/src/test/java/org/apache/druid/sql/calcite/expression/ExpressionsTest.java:
##########
@@ -198,127 +196,146 @@ public void testCharacterLength()
@Test
public void testSubstring()
{
- testHelper.testExpressionString(
+ testHelper.testExpression(
new SubstringOperatorConversion().calciteOperator(),
ImmutableList.of(
testHelper.makeInputRef("s"),
testHelper.makeLiteral(1),
testHelper.makeLiteral(2)
),
- makeExpression(
- SimpleExtraction.of("s", new SubstringDimExtractionFn(0, 2)),
- "substring(\"s\", 0, 2)"
+ DruidExpression.ofColumn(ColumnType.STRING, "s").map(
+ extraction -> null,
+ expression -> "substring(\"s\", 0, 2)"
),
"fo"
);
- testHelper.testExpressionString(
+ testHelper.testExpression(
new SubstringOperatorConversion().calciteOperator(),
ImmutableList.of(
testHelper.makeInputRef("s"),
testHelper.makeLiteral(2),
testHelper.makeLiteral(1)
),
- makeExpression(
- SimpleExtraction.of("s", new SubstringDimExtractionFn(1, 1)),
- "substring(\"s\", 1, 1)"
+ DruidExpression.ofColumn(ColumnType.STRING, "s").map(
+ extraction -> null,
+ expression -> "substring(\"s\", 1, 1)"
),
"o"
);
- testHelper.testExpressionString(
+ testHelper.testExpression(
new SubstringOperatorConversion().calciteOperator(),
ImmutableList.of(
testHelper.makeInputRef("s"),
testHelper.makeLiteral(1)
),
- makeExpression(
- SimpleExtraction.of("s", new SubstringDimExtractionFn(0, null)),
- "substring(\"s\", 0, -1)"
+ DruidExpression.ofColumn(ColumnType.STRING, "s").map(
+ extraction -> null,
+ expression -> "substring(\"s\", 0, -1)"
),
"foo"
);
- testHelper.testExpressionString(
+ testHelper.testExpression(
new SubstringOperatorConversion().calciteOperator(),
ImmutableList.of(
testHelper.makeInputRef("s"),
testHelper.makeLiteral(2)
),
- makeExpression(
- SimpleExtraction.of("s", new SubstringDimExtractionFn(1, null)),
- "substring(\"s\", 1, -1)"
+ DruidExpression.ofColumn(ColumnType.STRING, "s").map(
+ extraction -> null,
+ expression -> "substring(\"s\", 1, -1)"
),
"oo"
);
- testHelper.testExpressionString(
+ testHelper.testExpression(
new SubstringOperatorConversion().calciteOperator(),
ImmutableList.of(
testHelper.makeInputRef("s"),
testHelper.makeLiteral(1),
testHelper.makeInputRef("p") // p is 3
),
- makeExpression("substring(\"s\", 0, \"p\")"),
+ DruidExpression.ofColumn(ColumnType.STRING, "s").map(
+ extraction -> null,
+ expression -> "substring(\"s\", 0, \"p\")"
+ ),
"foo"
);
- testHelper.testExpressionString(
+ testHelper.testExpression(
new SubstringOperatorConversion().calciteOperator(),
ImmutableList.of(
testHelper.makeInputRef("spacey"),
testHelper.makeInputRef("p") // p is 3
),
- makeExpression("substring(\"spacey\", (\"p\" - 1), -1)"),
+ DruidExpression.ofColumn(ColumnType.STRING, "spacey").map(
+ extraction -> null,
+ expression -> "substring(\"spacey\", (\"p\" - 1), -1)"
+ ),
"hey there "
);
- testHelper.testExpressionString(
+ testHelper.testExpression(
new SubstringOperatorConversion().calciteOperator(),
ImmutableList.of(
testHelper.makeInputRef("spacey"),
testHelper.makeInputRef("p"), // p is 3
testHelper.makeInputRef("p") // p is 3
),
- makeExpression("substring(\"spacey\", (\"p\" - 1), \"p\")"),
+ DruidExpression.ofColumn(ColumnType.STRING, "spacey").map(
+ extraction -> null,
+ expression -> "substring(\"spacey\", (\"p\" - 1), \"p\")"
+ ),
"hey"
);
}
@Test
public void testRegexpExtract()
{
- testHelper.testExpressionString(
+ testHelper.testExpression(
new RegexpExtractOperatorConversion().calciteOperator(),
ImmutableList.of(
testHelper.makeInputRef("s"),
testHelper.makeLiteral("x(.)"),
testHelper.makeLiteral(1)
),
- makeExpression(
- SimpleExtraction.of("s", new RegexDimExtractionFn("x(.)", 1, true,
null)),
- "regexp_extract(\"s\",'x(.)',1)"
+ DruidExpression.ofFunctionCall(
Review Comment:
Most of the hand-built trees in this file could use the helper you reworked,
e.g.
```java
testHelper.buildExpectedExpression(ColumnType.STRING, "regexp_extract",
testHelper.makeVariable("s"), "x(.)", 1)
```
builds the same thing as the 9 lines here. For nested calls like the
`concat` one below, if `toArgumentExpression` passes a `DruidExpression` arg
through as-is:
```java
} else if (arg instanceof DruidExpression) {
return (DruidExpression) arg;
}
```
then it becomes
```java
testHelper.buildExpectedExpression(
ColumnType.STRING,
"regexp_extract",
testHelper.buildExpectedExpression(ColumnType.STRING, "concat", "Z",
testHelper.makeVariable("s")),
"Zf(.)"
)
```
I tried this on the first three `regexp_extract` cases locally and
`ExpressionsTest` still passes. It'd cut a big chunk of the diff.
##########
sql/src/test/java/org/apache/druid/sql/calcite/expression/ExpressionTestHelper.java:
##########
@@ -244,35 +244,39 @@ private static class Variable
{
this.name = name;
}
-
- @Override
- public String toString()
- {
- return "\"" + name + "\"";
- }
}
- DruidExpression buildExpectedExpression(String functionName, Object... args)
+ /**
+ * Builds the expected {@link DruidExpression} of a function call. Each of
the {@code args} is converted to an
+ * argument expression: {@link #makeVariable(String)} becomes a column of
the type in the row signature, while null,
+ * {@link String}, {@link Boolean}, {@link Long}, {@link Integer} and {@link
Double} become literals.
+ */
+ DruidExpression buildExpectedExpression(ColumnType type, String
functionName, Object... args)
Review Comment:
Since this now always returns `ofFunctionCall`, how about renaming it to
`expectedFunctionCall`? It says what it builds, and keeps it clear it's for the
expected side, not the code under test.
##########
sql/src/test/java/org/apache/druid/sql/calcite/expression/ExpressionsTest.java:
##########
@@ -198,127 +196,146 @@ public void testCharacterLength()
@Test
public void testSubstring()
{
- testHelper.testExpressionString(
+ testHelper.testExpression(
new SubstringOperatorConversion().calciteOperator(),
ImmutableList.of(
testHelper.makeInputRef("s"),
testHelper.makeLiteral(1),
testHelper.makeLiteral(2)
),
- makeExpression(
- SimpleExtraction.of("s", new SubstringDimExtractionFn(0, 2)),
- "substring(\"s\", 0, 2)"
+ DruidExpression.ofColumn(ColumnType.STRING, "s").map(
Review Comment:
These expectations are built with `ofColumn(...).map(extraction -> null,
...)`, which is the same call `SubstringOperatorConversion` makes. If that
`map()` ever carries the type or arguments wrong, the expected value changes
the same way and the test still passes. Could we write the expected value out
directly instead?
Also, the extraction-fn branch (`SubstringDimExtractionFn`) isn't hit here
because `PLANNER_CONTEXT` has `sqlUseExtractionFns=false`. That was already
true before since the old check only compared strings, but now that the compare
is deep, it might be worth a case with extraction fns enabled. Same for
`RegexDimExtractionFn` in `testRegexpExtract`.
##########
sql/src/test/java/org/apache/druid/sql/calcite/expression/ExpressionTestHelper.java:
##########
@@ -244,35 +244,39 @@ private static class Variable
{
this.name = name;
}
-
- @Override
- public String toString()
- {
- return "\"" + name + "\"";
- }
}
- DruidExpression buildExpectedExpression(String functionName, Object... args)
+ /**
+ * Builds the expected {@link DruidExpression} of a function call. Each of
the {@code args} is converted to an
+ * argument expression: {@link #makeVariable(String)} becomes a column of
the type in the row signature, while null,
+ * {@link String}, {@link Boolean}, {@link Long}, {@link Integer} and {@link
Double} become literals.
+ */
+ DruidExpression buildExpectedExpression(ColumnType type, String
functionName, Object... args)
{
- String noDelimiter = "";
- String argsString = Arrays.stream(args)
- .map(ExpressionTestHelper::quoteIfNeeded)
- .collect(Collectors.joining(","));
- List<String> elements = Arrays.asList(functionName, "(", argsString, ")");
- return CalciteTestBase.makeExpression(String.join(noDelimiter, elements));
+ return DruidExpression.ofFunctionCall(
+ type,
+ functionName,
+
Arrays.stream(args).map(this::toArgumentExpression).collect(Collectors.toList())
+ );
}
- private static String quoteIfNeeded(@Nullable Object arg)
+ private DruidExpression toArgumentExpression(@Nullable Object arg)
{
if (arg == null) {
- return "null";
+ return DruidExpression.ofLiteral(ColumnType.STRING,
DruidExpression.nullLiteral());
+ } else if (arg instanceof Variable) {
+ final String name = ((Variable) arg).name;
+ return
DruidExpression.ofColumn(rowSignature.getColumnType(name).orElse(null), name);
Review Comment:
If the name isn't in the row signature (say, a typo in `makeVariable`), this
silently gives a column with a null type. Throwing here would make that mistake
much easier to spot.
##########
sql/src/test/java/org/apache/druid/sql/calcite/expression/ExpressionsTest.java:
##########
@@ -714,92 +927,153 @@ public void testRegexpLikeAsFilter()
@Test
public void testStringFormat()
{
- testHelper.testExpressionString(
+ testHelper.testExpression(
new StringFormatOperatorConversion().calciteOperator(),
ImmutableList.of(
testHelper.makeLiteral("%x"),
testHelper.makeInputRef("b")
),
- makeExpression("format('%x',\"b\")"),
+ DruidExpression.ofFunctionCall(
+ ColumnType.STRING,
+ "format",
+ ImmutableList.of(
+ DruidExpression.ofLiteral(ColumnType.STRING, "'%x'"),
+ DruidExpression.ofColumn(ColumnType.LONG, "b")
+ )
+ ),
"19"
);
- testHelper.testExpressionString(
+ testHelper.testExpression(
new StringFormatOperatorConversion().calciteOperator(),
ImmutableList.of(
testHelper.makeLiteral("%s %,d"),
testHelper.makeInputRef("s"),
testHelper.makeLiteral(1234)
),
- makeExpression("format('%s %,d',\"s\",1234)"),
+ DruidExpression.ofFunctionCall(
+ ColumnType.STRING,
+ "format",
+ ImmutableList.of(
+ DruidExpression.ofLiteral(ColumnType.STRING, "'%s %,d'"),
+ DruidExpression.ofColumn(ColumnType.STRING, "s"),
+ DruidExpression.ofLiteral(ColumnType.LONG, "1234")
+ )
+ ),
"foo 1,234"
);
- testHelper.testExpressionString(
+ testHelper.testExpression(
new StringFormatOperatorConversion().calciteOperator(),
ImmutableList.of(
testHelper.makeLiteral("%s %,d"),
testHelper.makeInputRef("s")
),
- makeExpression("format('%s %,d',\"s\")"),
+ DruidExpression.ofFunctionCall(
+ ColumnType.STRING,
+ "format",
+ ImmutableList.of(
+ DruidExpression.ofLiteral(ColumnType.STRING, "'%s %,d'"),
+ DruidExpression.ofColumn(ColumnType.STRING, "s")
+ )
+ ),
"%s %,d; foo"
);
- testHelper.testExpressionString(
+ testHelper.testExpression(
new StringFormatOperatorConversion().calciteOperator(),
ImmutableList.of(
testHelper.makeLiteral("%s %,d"),
testHelper.makeInputRef("s"),
testHelper.makeLiteral(1234),
testHelper.makeLiteral(6789)
),
- makeExpression("format('%s %,d',\"s\",1234,6789)"),
+ DruidExpression.ofFunctionCall(
+ ColumnType.STRING,
+ "format",
+ ImmutableList.of(
+ DruidExpression.ofLiteral(ColumnType.STRING, "'%s %,d'"),
+ DruidExpression.ofColumn(ColumnType.STRING, "s"),
+ DruidExpression.ofLiteral(ColumnType.LONG, "1234"),
+ DruidExpression.ofLiteral(ColumnType.LONG, "6789")
+ )
+ ),
"foo 1,234"
);
- testHelper.testExpressionString(
+ testHelper.testExpression(
new StringFormatOperatorConversion().calciteOperator(),
ImmutableList.of(
testHelper.makeLiteral("%.2f %.2f %.2f"),
testHelper.makeLiteral(1234d),
testHelper.makeLiteral(123.4),
testHelper.makeLiteral(123.456789)
),
- makeExpression("format('%.2f %.2f %.2f',1234.0,123.4,123.456789)"),
+ DruidExpression.ofFunctionCall(
+ ColumnType.STRING,
+ "format",
+ ImmutableList.of(
+ DruidExpression.ofLiteral(ColumnType.STRING, "'%.2f %.2f
%.2f'"),
+ DruidExpression.ofLiteral(ColumnType.DOUBLE, "1234.0"),
+ DruidExpression.ofLiteral(ColumnType.DOUBLE, "123.4"),
+ DruidExpression.ofLiteral(ColumnType.DOUBLE, "123.456789")
+ )
+ ),
"1234.00 123.40 123.46"
);
}
@Test
public void testStrpos()
{
- testHelper.testExpressionString(
+ testHelper.testExpression(
new StrposOperatorConversion().calciteOperator(),
ImmutableList.of(
testHelper.makeInputRef("s"),
testHelper.makeLiteral("oo")
),
- makeExpression("(strpos(\"s\",'oo') + 1)"),
+ DruidExpression.ofExpression(
+ ColumnType.LONG,
+ (args) -> "(strpos(\"s\",'oo') + 1)",
Review Comment:
Minor: the lambda ignores `args`, so the string and the argument list aren't
tied together. Nothing stops them from disagreeing (e.g. `'oo'` in the string,
`'ax'` in the args). Not a blocker, just something to watch for since this
pattern repeats a lot.
##########
sql/src/test/java/org/apache/druid/sql/calcite/expression/ExpressionTestHelper.java:
##########
@@ -244,35 +244,39 @@ private static class Variable
{
this.name = name;
}
-
- @Override
- public String toString()
- {
- return "\"" + name + "\"";
- }
}
- DruidExpression buildExpectedExpression(String functionName, Object... args)
+ /**
+ * Builds the expected {@link DruidExpression} of a function call. Each of
the {@code args} is converted to an
+ * argument expression: {@link #makeVariable(String)} becomes a column of
the type in the row signature, while null,
+ * {@link String}, {@link Boolean}, {@link Long}, {@link Integer} and {@link
Double} become literals.
+ */
+ DruidExpression buildExpectedExpression(ColumnType type, String
functionName, Object... args)
{
- String noDelimiter = "";
- String argsString = Arrays.stream(args)
- .map(ExpressionTestHelper::quoteIfNeeded)
- .collect(Collectors.joining(","));
- List<String> elements = Arrays.asList(functionName, "(", argsString, ")");
- return CalciteTestBase.makeExpression(String.join(noDelimiter, elements));
+ return DruidExpression.ofFunctionCall(
+ type,
+ functionName,
+
Arrays.stream(args).map(this::toArgumentExpression).collect(Collectors.toList())
+ );
}
- private static String quoteIfNeeded(@Nullable Object arg)
+ private DruidExpression toArgumentExpression(@Nullable Object arg)
{
if (arg == null) {
- return "null";
+ return DruidExpression.ofLiteral(ColumnType.STRING,
DruidExpression.nullLiteral());
Review Comment:
Nulls are always typed `STRING` here, but elsewhere in `ExpressionsTest` a
null literal is expected as `ofLiteral(null, "null")` (e.g. `timestamp_floor`).
It works for the current Greatest/Least cases, but a typed null (BIGINT,
TIMESTAMP) would fail with a confusing diff. Maybe let the caller pass the
type, or note the limitation in the javadoc?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
