You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "FrankChen021 (via GitHub)" <gi...@apache.org> on 2026/10/24 15:07:41 UTC
Re: [PR] test: remove deprecated testExpressionString from SQL expression tests (druid)
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org