From 6ab8d920e00e7c5ffd2ba72c82dad371a6dfd3aa Mon Sep 17 00:00:00 2001 From: Louis Chu Date: Fri, 24 Jul 2026 00:34:34 +0800 Subject: [PATCH 1/5] Normalize CHAR/VARCHAR literals to Str in DataFusion VirtualTable conversion isthmus's visit(Values) types each schema column from the Calcite RelDataType but derives each row literal from the string value, emitting fixed_char(n). A multi-row literal table (e.g. makeresults data=) then trips VirtualTableScan's row-conforms-to-schema check (FixedChar(3) vs FixedChar(5), or FixedChar vs Str). That check runs during isthmus POJO construction, before any proto is produced, so a proto-level rewrite is too late. Override visit(Values) on the SubstraitRelVisitor subclass in createVisitor and build the VirtualTableScan directly, mapping every CHAR/VARCHAR column to a length-independent Str in both the base schema and every row literal. Numeric and boolean columns are left to isthmus's own LiteralConverter and TypeConverter. Empty (zero-row) Values delegate to super.visit unchanged. Signed-off-by: Louis Chu --- .../DataFusionFragmentConvertor.java | 73 +++++++++++++++++++ 1 file changed, 73 insertions(+) diff --git a/sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionFragmentConvertor.java b/sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionFragmentConvertor.java index 4e265ff0d09b6..00ebdb165d718 100644 --- a/sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionFragmentConvertor.java +++ b/sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionFragmentConvertor.java @@ -61,6 +61,7 @@ import io.substrait.expression.AggregateFunctionInvocation; import io.substrait.expression.Expression; +import io.substrait.expression.ExpressionCreator; import io.substrait.expression.FunctionArg; import io.substrait.expression.ImmutableAggregateFunctionInvocation; import io.substrait.extension.ExtensionCollector; @@ -70,6 +71,7 @@ import io.substrait.isthmus.TypeConverter; import io.substrait.isthmus.expression.AggregateFunctionConverter; import io.substrait.isthmus.expression.FunctionMappings; +import io.substrait.isthmus.expression.LiteralConverter; import io.substrait.isthmus.expression.ScalarFunctionConverter; import io.substrait.isthmus.expression.WindowFunctionConverter; import io.substrait.plan.Plan; @@ -83,8 +85,10 @@ import io.substrait.relation.Project; import io.substrait.relation.Rel; import io.substrait.relation.Sort; +import io.substrait.relation.VirtualTableScan; import io.substrait.type.NamedStruct; import io.substrait.type.Type; +import io.substrait.type.TypeCreator; import io.substrait.type.proto.TypeProtoConverter; /** Converts Calcite RelNode fragments to Substrait protobuf bytes for the DataFusion Rust runtime. */ @@ -769,15 +773,84 @@ protected ImmutableList getSigs() { windowConverter, typeConverter ); + LiteralConverter literalConverter = new LiteralConverter(typeConverter); return new SubstraitRelVisitor(converterProvider) { @Override public Rel visit(org.apache.calcite.rel.core.Aggregate aggregate) { Rel rel = super.visit(aggregate); return rel instanceof Aggregate agg ? addNullArgFilters(aggregate, agg) : rel; } + + @Override + public Rel visit(org.apache.calcite.rel.core.Values values) { + if (values.getTuples().isEmpty()) { + // No row literals means no per-value char length to conflict with the schema; + // isthmus's default zero-row VirtualTable is fine. + return super.visit(values); + } + return virtualTableWithStrLiterals(values, typeConverter, literalConverter); + } }; } + /** + * Builds a Substrait {@link VirtualTableScan} for an inline-literal {@link org.apache.calcite.rel.core.Values} + * leaf, mapping every CHAR/VARCHAR column to a length-independent {@code Str} in BOTH the base + * schema and every row literal. isthmus's own {@code visit(Values)} derives a + * {@code fixed_char(n)} literal from each string value while typing the schema column from the + * RelDataType, so a multi-row literal table (e.g. {@code makeresults data=}) trips + * {@link VirtualTableScan}'s row-conforms-to-schema check (FixedChar(3) vs FixedChar(5), or + * FixedChar vs Str). Normalising char types to {@code Str} here at the RelNode-to-POJO layer, in + * front of that check, makes the VirtualTable self-consistent. Numeric and boolean columns are + * left to isthmus's own literal/type converters since they are already length-independent. + */ + private static Rel virtualTableWithStrLiterals( + org.apache.calcite.rel.core.Values values, + TypeConverter typeConverter, + LiteralConverter literalConverter + ) { + RelDataType rowType = values.getRowType(); + List fields = rowType.getFieldList(); + + List names = new ArrayList<>(fields.size()); + List fieldTypes = new ArrayList<>(fields.size()); + for (RelDataTypeField f : fields) { + names.add(f.getName()); + if (isCharacter(f.getType())) { + fieldTypes.add(TypeCreator.of(f.getType().isNullable()).STRING); + } else { + fieldTypes.add(typeConverter.toSubstrait(f.getType())); + } + } + NamedStruct schema = NamedStruct.of(names, TypeCreator.REQUIRED.struct(fieldTypes)); + + List rows = new ArrayList<>(values.getTuples().size()); + for (List tuple : values.getTuples()) { + List cells = new ArrayList<>(tuple.size()); + for (int c = 0; c < tuple.size(); c++) { + RexLiteral lit = tuple.get(c); + RelDataType cellType = fields.get(c).getType(); + if (isCharacter(cellType)) { + if (lit.isNull()) { + cells.add(ExpressionCreator.typedNull(TypeCreator.of(true).STRING)); + } else { + cells.add(ExpressionCreator.string(cellType.isNullable(), lit.getValueAs(String.class))); + } + } else { + cells.add(literalConverter.convert(lit)); + } + } + rows.add(ExpressionCreator.nestedStruct(false, cells)); + } + + return VirtualTableScan.builder().initialSchema(schema).addAllRows(rows).build(); + } + + private static boolean isCharacter(RelDataType type) { + SqlTypeName t = type.getSqlTypeName(); + return t == SqlTypeName.CHAR || t == SqlTypeName.VARCHAR; + } + /** * Adds an {@code is_not_null} {@code preMeasureFilter} to each measure whose {@link LocalAggOp} * declares {@link LocalAggOp#filtersNullArgs} — so the converter stays generic and only the op From 2e6d7774032b7fb821a99840691dd251d79a7505 Mon Sep 17 00:00:00 2001 From: Louis Chu Date: Thu, 30 Jul 2026 23:24:06 +0800 Subject: [PATCH 2/5] Trim visit(Values) javadoc to essentials Signed-off-by: Louis Chu --- .../be/datafusion/DataFusionFragmentConvertor.java | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionFragmentConvertor.java b/sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionFragmentConvertor.java index 00ebdb165d718..23f24fde94c76 100644 --- a/sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionFragmentConvertor.java +++ b/sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionFragmentConvertor.java @@ -794,15 +794,9 @@ public Rel visit(org.apache.calcite.rel.core.Values values) { } /** - * Builds a Substrait {@link VirtualTableScan} for an inline-literal {@link org.apache.calcite.rel.core.Values} - * leaf, mapping every CHAR/VARCHAR column to a length-independent {@code Str} in BOTH the base - * schema and every row literal. isthmus's own {@code visit(Values)} derives a - * {@code fixed_char(n)} literal from each string value while typing the schema column from the - * RelDataType, so a multi-row literal table (e.g. {@code makeresults data=}) trips - * {@link VirtualTableScan}'s row-conforms-to-schema check (FixedChar(3) vs FixedChar(5), or - * FixedChar vs Str). Normalising char types to {@code Str} here at the RelNode-to-POJO layer, in - * front of that check, makes the VirtualTable self-consistent. Numeric and boolean columns are - * left to isthmus's own literal/type converters since they are already length-independent. + * Builds the Substrait {@link VirtualTableScan} for an inline {@link org.apache.calcite.rel.core.Values} + * leaf, normalising every CHAR/VARCHAR column to a length-independent {@code Str} in both the base + * schema and the row literals. */ private static Rel virtualTableWithStrLiterals( org.apache.calcite.rel.core.Values values, From efeee4b4e1d92315899cb41dbb0ba04a7824ffbc Mon Sep 17 00:00:00 2001 From: Louis Chu Date: Fri, 31 Jul 2026 00:01:21 +0800 Subject: [PATCH 3/5] Add unit tests for VirtualTable CHAR/VARCHAR to Str normalization Signed-off-by: Louis Chu --- .../DataFusionFragmentConvertorTests.java | 64 +++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/sandbox/plugins/analytics-backend-datafusion/src/test/java/org/opensearch/be/datafusion/DataFusionFragmentConvertorTests.java b/sandbox/plugins/analytics-backend-datafusion/src/test/java/org/opensearch/be/datafusion/DataFusionFragmentConvertorTests.java index 3c5e4b723822d..faac388118dd2 100644 --- a/sandbox/plugins/analytics-backend-datafusion/src/test/java/org/opensearch/be/datafusion/DataFusionFragmentConvertorTests.java +++ b/sandbox/plugins/analytics-backend-datafusion/src/test/java/org/opensearch/be/datafusion/DataFusionFragmentConvertorTests.java @@ -36,6 +36,9 @@ import org.opensearch.analytics.planner.rel.OpenSearchStageInputScan; import org.opensearch.analytics.spi.DelegatedPredicateFunction; import org.opensearch.test.OpenSearchTestCase; +import org.apache.calcite.rel.logical.LogicalValues; +import org.apache.calcite.rex.RexLiteral; +import com.google.common.collect.ImmutableList; import java.util.List; @@ -849,4 +852,65 @@ public void testAttachFragmentOnTop_PreservesLiftedWindowProjectLayer() throws E assertTrue("lower's input must be the rewired stage-scan", innerOfLower.hasRead()); } + + // -- VirtualTable (inline Values) CHAR/VARCHAR -> Str normalization (PR #22554) -- + + /** + * A Values with a character column whose rows differ in length must convert to a VirtualTable + * whose schema column AND every row cell are Str, not fixed_char. Before the fix this tripped + * VirtualTableScan's row-conforms-to-schema check (FixedChar(5) vs FixedChar(3)) and + * convertFragment threw IllegalStateException. + */ + public void testVirtualTable_MultiLengthCharColumn_NormalizedToStr() throws Exception { + LogicalValues values = charValues("Alice", "Bob"); + ReadRel read = rootRel(decodeSubstrait(newConvertor().convertFragment(values))).getRead(); + assertTrue("must be a VirtualTable", read.hasVirtualTable()); + assertTrue("char schema column must be Str, not fixed_char", read.getBaseSchema().getStruct().getTypes(0).hasString()); + assertEquals("two rows", 2, read.getVirtualTable().getExpressionsCount()); + for (int r = 0; r < 2; r++) { + Expression cell = read.getVirtualTable().getExpressions(r).getFields(0); + assertTrue("row " + r + " cell must be a string literal", cell.getLiteral().hasString()); + } + } + + /** A mixed character + integer Values: char column -> Str; the integer column is left to isthmus and stays i32. */ + public void testVirtualTable_MixedCharAndInt_IntStaysNumeric() throws Exception { + RelDataType charType = typeFactory.createTypeWithNullability(typeFactory.createSqlType(SqlTypeName.CHAR, 5), true); + RelDataType intType = typeFactory.createSqlType(SqlTypeName.INTEGER); + RelDataType rowType = typeFactory.builder().add("name", charType).add("age", intType).build(); + ImmutableList> tuples = ImmutableList.of( + ImmutableList.of(charLiteral("Alice"), (RexLiteral) rexBuilder.makeLiteral(30, intType, false)), + ImmutableList.of(charLiteral("Bob"), (RexLiteral) rexBuilder.makeLiteral(25, intType, false))); + LogicalValues values = (LogicalValues) LogicalValues.create(cluster, rowType, tuples); + ReadRel read = rootRel(decodeSubstrait(newConvertor().convertFragment(values))).getRead(); + assertTrue("char column -> Str", read.getBaseSchema().getStruct().getTypes(0).hasString()); + assertTrue("int column stays numeric (i32)", read.getBaseSchema().getStruct().getTypes(1).hasI32()); + Expression.Nested.Struct row0 = read.getVirtualTable().getExpressions(0); + assertTrue("char cell -> string literal", row0.getFields(0).getLiteral().hasString()); + assertEquals("int cell stays i32", 30, row0.getFields(1).getLiteral().getI32()); + } + + /** A zero-row Values takes the guarded branch (super.visit) and still lowers to a ReadRel. */ + public void testVirtualTable_EmptyValues_DelegatesToSuper() throws Exception { + RelDataType charType = typeFactory.createTypeWithNullability(typeFactory.createSqlType(SqlTypeName.CHAR, 5), true); + RelDataType rowType = typeFactory.builder().add("name", charType).build(); + LogicalValues empty = (LogicalValues) LogicalValues.createEmpty(cluster, rowType); + assertTrue("empty Values still lowers to a ReadRel", rootRel(decodeSubstrait(newConvertor().convertFragment(empty))).hasRead()); + } + + /** Builds a single-column CHAR Values, one row per value (each CHAR of the value's own length). */ + private LogicalValues charValues(String... values) { + RelDataType colType = typeFactory.createTypeWithNullability(typeFactory.createSqlType(SqlTypeName.CHAR, 5), true); + RelDataType rowType = typeFactory.builder().add("name", colType).build(); + java.util.List> tuples = new java.util.ArrayList<>(); + for (String v : values) { + tuples.add(ImmutableList.of(charLiteral(v))); + } + return (LogicalValues) LogicalValues.create(cluster, rowType, ImmutableList.copyOf(tuples)); + } + + /** A CHAR literal of the value's own length (e.g. 'Alice' -> CHAR(5), 'Bob' -> CHAR(3)). */ + private RexLiteral charLiteral(String v) { + return (RexLiteral) rexBuilder.makeLiteral(v, typeFactory.createSqlType(SqlTypeName.CHAR, v.length()), false); + } } From 4e3d1309c5f978b3fc65dd429ea201d24a0a2803 Mon Sep 17 00:00:00 2001 From: Louis Chu Date: Fri, 31 Jul 2026 00:17:12 +0800 Subject: [PATCH 4/5] Add extensive_coverage IT: makeresults on the analytics engine (supported) Signed-off-by: Louis Chu --- .../src/test/resources/datasets/extensive_coverage/ppl/q198.ppl | 1 + 1 file changed, 1 insertion(+) create mode 100644 sandbox/qa/analytics-engine-rest/src/test/resources/datasets/extensive_coverage/ppl/q198.ppl diff --git a/sandbox/qa/analytics-engine-rest/src/test/resources/datasets/extensive_coverage/ppl/q198.ppl b/sandbox/qa/analytics-engine-rest/src/test/resources/datasets/extensive_coverage/ppl/q198.ppl new file mode 100644 index 0000000000000..b63ff48faec4d --- /dev/null +++ b/sandbox/qa/analytics-engine-rest/src/test/resources/datasets/extensive_coverage/ppl/q198.ppl @@ -0,0 +1 @@ +makeresults format=json data='[{"name":"Alice","age":"30"},{"name":"Bob","age":"25"}]' From 0663a61f7472a2c1d3994017571132c13b2dd0e1 Mon Sep 17 00:00:00 2001 From: Louis Chu Date: Fri, 31 Jul 2026 00:29:57 +0800 Subject: [PATCH 5/5] Add datarows golden for q198 makeresults IT (project away @timestamp via fields) Signed-off-by: Louis Chu --- .../extensive_coverage/ppl/expected/q198.json | 12 ++++++++++++ .../datasets/extensive_coverage/ppl/q198.ppl | 2 +- 2 files changed, 13 insertions(+), 1 deletion(-) create mode 100644 sandbox/qa/analytics-engine-rest/src/test/resources/datasets/extensive_coverage/ppl/expected/q198.json diff --git a/sandbox/qa/analytics-engine-rest/src/test/resources/datasets/extensive_coverage/ppl/expected/q198.json b/sandbox/qa/analytics-engine-rest/src/test/resources/datasets/extensive_coverage/ppl/expected/q198.json new file mode 100644 index 0000000000000..748ba52dad6d3 --- /dev/null +++ b/sandbox/qa/analytics-engine-rest/src/test/resources/datasets/extensive_coverage/ppl/expected/q198.json @@ -0,0 +1,12 @@ +{ + "schema": [ + { "name": "name", "type": "string" }, + { "name": "age", "type": "string" } + ], + "datarows": [ + [ "Alice", "30" ], + [ "Bob", "25" ] + ], + "total": 2, + "size": 2 +} diff --git a/sandbox/qa/analytics-engine-rest/src/test/resources/datasets/extensive_coverage/ppl/q198.ppl b/sandbox/qa/analytics-engine-rest/src/test/resources/datasets/extensive_coverage/ppl/q198.ppl index b63ff48faec4d..04089d400d5c8 100644 --- a/sandbox/qa/analytics-engine-rest/src/test/resources/datasets/extensive_coverage/ppl/q198.ppl +++ b/sandbox/qa/analytics-engine-rest/src/test/resources/datasets/extensive_coverage/ppl/q198.ppl @@ -1 +1 @@ -makeresults format=json data='[{"name":"Alice","age":"30"},{"name":"Bob","age":"25"}]' +makeresults format=json data='[{"name":"Alice","age":"30"},{"name":"Bob","age":"25"}]' | fields name, age