Skip to content

[analytics-backend-datafusion] Normalize CHAR/VARCHAR literals to Str in VirtualTable conversion - #22554

Open
noCharger wants to merge 6 commits into
opensearch-project:mainfrom
noCharger:fix/datafusion-virtualtable-char-normalization
Open

[analytics-backend-datafusion] Normalize CHAR/VARCHAR literals to Str in VirtualTable conversion#22554
noCharger wants to merge 6 commits into
opensearch-project:mainfrom
noCharger:fix/datafusion-virtualtable-char-normalization

Conversation

@noCharger

@noCharger noCharger commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Description

An inline literal-row plan (a Calcite LogicalValues with string columns, e.g. from
PPL makeresults format=csv data=...) fails to convert to Substrait for the
DataFusion runtime. When a string column has rows of differing length it throws,
during Substrait POJO construction,
AssertionError: Row field type (FixedChar{length=3}) does not match schema field type (FixedChar{length=5}),
which the convertor rewraps as IllegalStateException: Substrait conversion rejected the plan: ....

Root cause. isthmus types each row's string literal from the value itself
(FixedCharLiteral.getType() returns fixedChar(value().length()), so 'Bob' is
FixedChar(3)), while the column schema comes from the Calcite CHAR(maxLen) type
(FixedChar(5)). VirtualTableScan's @Value.Check then asserts each cell type is
exactly equal to the column type, with no widening, so a shorter cell is rejected.
This runs at the RelNode-to-POJO layer, before any proto is generated, so a
proto-level rewrite is too late; and the CAST(...):VARCHAR that the makeresults
Project adds sits above the Values and does not help. (Equal-length rows produce
matching FixedChar(n) on both sides and are unaffected.)

Fix. Override visit(Values) on the SubstraitRelVisitor subclass in
createVisitor (the seam already used for visit(Aggregate)) and map every
CHAR/VARCHAR column to a length-independent Str in both the base schema and every
row literal (ExpressionCreator.string). Numeric/boolean columns are left to
isthmus; empty (zero-row) Values delegate to super.visit. This is a general fix for
any inline-literal Values routed to the DataFusion backend, not specific to
makeresults.

Reproduction

On a cluster with cluster.pluggable.dataformat: composite (so scan-less
makeresults routes to the analytics engine):

POST /_plugins/_ppl
{"query": "makeresults format=csv data='name,age\nAlice,30\nBob,25'"}

Logical plan (_explain); the LogicalValues cells are CHAR(n), which is why the
Project inserts CAST($n):VARCHAR:

LogicalProject(name=[CAST($0):VARCHAR NOT NULL], age=[CAST($1):VARCHAR NOT NULL])
  LogicalValues(tuples=[[{ 'Alice', '30' }, { 'Bob', '25' }]])
  • Before: HTTP 500, the AssertionError above (thrown at VirtualTableScan.validateRowConformsToSchema).
  • After: HTTP 200, datarows: [["Alice","30"],["Bob","25"]].
Full stack trace (before the fix)
java.lang.IllegalStateException: Substrait conversion rejected the plan: Row field type (FixedChar{nullable=false, length=3}) does not match schema field type (FixedChar{nullable=false, length=5})
	at org.opensearch.be.datafusion.DataFusionFragmentConvertor.convertToSubstrait(DataFusionFragmentConvertor.java:569)
	at org.opensearch.be.datafusion.DataFusionFragmentConvertor.convertFragment(DataFusionFragmentConvertor.java:473)
	at org.opensearch.analytics.planner.dag.FragmentConversionDriver.convert(FragmentConversionDriver.java:456)
	at org.opensearch.analytics.planner.dag.FragmentConversionDriver.convertStage(FragmentConversionDriver.java:122)
	at org.opensearch.analytics.planner.dag.FragmentConversionDriver.convertAll(FragmentConversionDriver.java:84)
	at org.opensearch.analytics.exec.DefaultPlanExecutor.executeInternal(DefaultPlanExecutor.java:269)
	... (analytics planner + thread-pool frames) ...
Caused by: java.lang.AssertionError: Row field type (FixedChar{nullable=false, length=3}) does not match schema field type (FixedChar{nullable=false, length=5})
	at io.substrait.relation.VirtualTableScan.validateRowConformsToSchema(VirtualTableScan.java:78)
	at io.substrait.relation.VirtualTableScan.check(VirtualTableScan.java:53)
	at io.substrait.relation.ImmutableVirtualTableScan$Builder.build(ImmutableVirtualTableScan.java:1106)
	at io.substrait.isthmus.SubstraitRelVisitor.visit(SubstraitRelVisitor.java:175)
	... (isthmus visitor recursion) ...
	at org.opensearch.be.datafusion.DataFusionFragmentConvertor.convertToSubstrait(DataFusionFragmentConvertor.java:565)
Source references (substrait-java 0.89.1)
  • isthmus/.../expression/LiteralConverter.javaCHAR -> ExpressionCreator.fixedChar(value); VARCHAR without precision -> ExpressionCreator.string (Str).
  • core/.../expression/Expression.javaFixedCharLiteral.getType() returns fixedChar(value().length()); StrLiteral.getType() returns STRING.
  • core/.../relation/VirtualTableScan.javacheck() / validateRowConformsToSchema() assert rowFieldType.equals(schemaFieldType).

Testing

  • :sandbox:plugins:analytics-backend-datafusion:test passing.
  • Verified end to end on a DataFusion cluster (cluster.pluggable.dataformat=composite):
    the query above returns HTTP 200 after the change and the assertion before it; a
    mixed string/int literal table also converts, with the int column staying numeric.
  • analytics-engine-rest IT suite shows no new failures; the only failures are
    pre-existing on main (FilterDelegationGoldenIT, SystemFunctionsIT.testTypeofArithmetic).

Check List

  • Functionality includes testing.
  • API changes companion pull request created, if applicable.
  • Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@github-actions

github-actions Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit 830d097)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review

Schema Nullability

The base schema struct is built as TypeCreator.REQUIRED.struct(fieldTypes), forcing the outer struct to be non-nullable regardless of the source row type. While individual field nullability is preserved, this may diverge from isthmus's default handling for Values and could affect downstream consumers that inspect struct-level nullability. Confirm this matches the intended contract.

NamedStruct schema = NamedStruct.of(names, TypeCreator.REQUIRED.struct(fieldTypes));
Row Nullability Mismatch

For non-null CHAR/VARCHAR cells, ExpressionCreator.string(cellType.isNullable(), ...) uses the cell's declared nullability, but for null cells the code hardcodes TypeCreator.of(true).STRING. If a nullable column contains a mix of null and non-null literals with the same schema nullability, the row-cell types may still disagree with the schema column type (nullable string vs non-nullable string) when the column is declared non-nullable but a null literal is emitted. Consider using the field's nullability consistently.

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)));
    }

@github-actions

github-actions Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Latest suggestions up to 830d097

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Align null literal nullability with schema

The nullability of each cell literal should be consistent with the schema column's
nullability (derived from fields.get(c).getType().isNullable()), not the per-literal
cellType.isNullable(), which may differ from the column's declared type. Using the
column nullability guarantees the row literal conforms to the schema struct.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionFragmentConvertor.java [828-832]

 if (lit.isNull()) {
-        cells.add(ExpressionCreator.typedNull(TypeCreator.of(true).STRING));
+        cells.add(ExpressionCreator.typedNull(TypeCreator.of(cellType.isNullable()).STRING));
     } else {
         cells.add(ExpressionCreator.string(cellType.isNullable(), lit.getValueAs(String.class)));
     }
Suggestion importance[1-10]: 5

__

Why: The suggestion has some merit: when the schema column is derived as TypeCreator.of(f.getType().isNullable()).STRING, using a hardcoded TypeCreator.of(true).STRING for typedNull could technically mismatch a non-nullable column. However, in practice a null literal implies nullable, and Calcite typically produces nullable types for null literals, so the impact is limited.

Low
Preserve struct nullability from row type

Using TypeCreator.REQUIRED.struct(...) forces the outer struct to be non-nullable,
which may not match how isthmus normally builds Values schemas (typically derived
from the row type's nullability). Consider deriving struct nullability from
rowType.isNullable() to stay consistent with the default super.visit(values) path
used for empty tuples.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionFragmentConvertor.java [819]

-NamedStruct schema = NamedStruct.of(names, TypeCreator.REQUIRED.struct(fieldTypes));
+NamedStruct schema = NamedStruct.of(names, TypeCreator.of(rowType.isNullable()).struct(fieldTypes));
Suggestion importance[1-10]: 3

__

Why: The suggestion raises a reasonable concern about consistency with isthmus's default behavior, but row types for Values are typically non-nullable structs, so the practical impact is minimal and could potentially introduce a regression if not carefully validated.

Low

Previous suggestions

Suggestions up to commit 0663a61
CategorySuggestion                                                                                                                                    Impact
Possible issue
Align literal nullability with schema field

The non-null branch uses cellType.isNullable() (per-cell nullability) while the
schema uses the field type's nullability. If any row's cellType differs in
nullability from the schema field type, the row won't conform to the schema. Use the
field type's nullability consistently to match the declared schema.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionFragmentConvertor.java [827-835]

 if (isCharacter(cellType)) {
+    boolean fieldNullable = fields.get(c).getType().isNullable();
     if (lit.isNull()) {
         cells.add(ExpressionCreator.typedNull(TypeCreator.of(true).STRING));
     } else {
-        cells.add(ExpressionCreator.string(cellType.isNullable(), lit.getValueAs(String.class)));
+        cells.add(ExpressionCreator.string(fieldNullable, lit.getValueAs(String.class)));
     }
 } else {
Suggestion importance[1-10]: 6

__

Why: Valid concern: the schema uses f.getType().isNullable() while the non-null cell uses cellType.isNullable(), which could mismatch if per-row RexLiteral types differ from the field type. Moderate impact since in practice Values typically have consistent per-column types.

Low
General
Trim CHAR padding when normalising to Str

CHAR literals in Calcite are typically space-padded to their declared length (e.g.,
CHAR(5) 'Bob' becomes 'Bob '). Using lit.getValueAs(String.class) may return the
padded value, changing observable semantics when normalising to Str. Consider
trimming trailing spaces for CHAR (not VARCHAR) to preserve the intended string
value.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionFragmentConvertor.java [831]

 } else {
-    cells.add(ExpressionCreator.string(cellType.isNullable(), lit.getValueAs(String.class)));
+    String s = lit.getValueAs(String.class);
+    if (cellType.getSqlTypeName() == SqlTypeName.CHAR && s != null) {
+        int end = s.length();
+        while (end > 0 && s.charAt(end - 1) == ' ') end--;
+        s = s.substring(0, end);
+    }
+    cells.add(ExpressionCreator.string(cellType.isNullable(), s));
 }
Suggestion importance[1-10]: 5

__

Why: Legitimate semantic concern about CHAR space-padding behavior when converting to variable-length Str, though behavior depends on how Calcite stores the value and may or may not need trimming.

Low
Suggestions up to commit 4e3d130
CategorySuggestion                                                                                                                                    Impact
General
Align cell nullability with schema column

The per-cell nullability derived from cellType.isNullable() may diverge from the
schema's column nullability (which is taken from f.getType().isNullable() — the same
source, but for CHAR literals Calcite often marks the literal type as non-nullable
even when the column is nullable). Use the schema field's nullability for the
literal type so row cells conform to the declared schema type.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionFragmentConvertor.java [828-832]

 if (lit.isNull()) {
     cells.add(ExpressionCreator.typedNull(TypeCreator.of(true).STRING));
 } else {
-    cells.add(ExpressionCreator.string(cellType.isNullable(), lit.getValueAs(String.class)));
+    cells.add(ExpressionCreator.string(fields.get(c).getType().isNullable(), lit.getValueAs(String.class)));
 }
Suggestion importance[1-10]: 5

__

Why: Using the schema field's nullability for cell literals is a reasonable consistency improvement, ensuring row cells conform to the declared schema type. The impact is minor since Calcite typically aligns these, but it prevents subtle mismatches.

Low
Preserve row-type nullability in schema struct

Wrapping field types in a REQUIRED.struct(...) forces the outer struct to be
non-nullable, but the individual field types already carry their own nullability. If
Calcite's row type is nullable at the struct level, this would be lost; however more
importantly, isthmus's default builds the struct using the row type's nullability.
Consider deriving the struct nullability from rowType.isNullable() for consistency.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionFragmentConvertor.java [819]

-NamedStruct schema = NamedStruct.of(names, TypeCreator.REQUIRED.struct(fieldTypes));
+NamedStruct schema = NamedStruct.of(names, TypeCreator.of(rowType.isNullable()).struct(fieldTypes));
Suggestion importance[1-10]: 3

__

Why: Row types in Calcite are typically non-nullable at the struct level, so this change likely has no practical effect. It's a minor consistency improvement without clear evidence of a bug.

Low
Suggestions up to commit efeee4b
CategorySuggestion                                                                                                                                    Impact
Possible issue
Align cell nullability with schema column

The row cell's nullability should match the schema column's nullability (derived
from f.getType().isNullable()), not the per-literal cellType.isNullable(). Otherwise
a non-null literal in a nullable column may produce a non-nullable cell that fails
Substrait's row-conforms-to-schema check. Use the field's nullability consistently.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionFragmentConvertor.java [828-832]

+boolean colNullable = fields.get(c).getType().isNullable();
 if (lit.isNull()) {
     cells.add(ExpressionCreator.typedNull(TypeCreator.of(true).STRING));
 } else {
-    cells.add(ExpressionCreator.string(cellType.isNullable(), lit.getValueAs(String.class)));
+    cells.add(ExpressionCreator.string(colNullable, lit.getValueAs(String.class)));
 }
Suggestion importance[1-10]: 7

__

Why: Valid concern: using per-literal cellType.isNullable() (from the CHAR(n) literal's type) could mismatch the schema column's nullability derived from f.getType(), potentially failing Substrait's row-conforms-to-schema check. The fix improves consistency.

Medium
General
Preserve struct nullability from row type

The outer struct is built with TypeCreator.REQUIRED, which forces the struct itself
to be non-nullable regardless of the source rowType. If the row type is nullable
this can misrepresent the schema; derive the struct nullability from
rowType.isNullable() to preserve semantics.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionFragmentConvertor.java [819]

-NamedStruct schema = NamedStruct.of(names, TypeCreator.REQUIRED.struct(fieldTypes));
+NamedStruct schema = NamedStruct.of(names, TypeCreator.of(rowType.isNullable()).struct(fieldTypes));
Suggestion importance[1-10]: 3

__

Why: Minor semantic improvement; Calcite row types for Values are typically non-nullable structs, so the practical impact is small, but preserving nullability from rowType is more correct.

Low
Suggestions up to commit 2e6d777
CategorySuggestion                                                                                                                                    Impact
Possible issue
Align literal nullability with schema

The nullability of the typed null and the string literal should match the schema
field's nullability derived from fields.get(c).getType().isNullable(). Currently the
null branch hard-codes true which can conflict with a non-nullable schema field, and
the non-null branch uses cellType.isNullable() inconsistently. Use the same
nullability for both branches to keep row literals aligned with the declared schema.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionFragmentConvertor.java [828-832]

+boolean nullable = cellType.isNullable();
 if (lit.isNull()) {
-    cells.add(ExpressionCreator.typedNull(TypeCreator.of(true).STRING));
+    cells.add(ExpressionCreator.typedNull(TypeCreator.of(nullable).STRING));
 } else {
-    cells.add(ExpressionCreator.string(cellType.isNullable(), lit.getValueAs(String.class)));
+    cells.add(ExpressionCreator.string(nullable, lit.getValueAs(String.class)));
 }
Suggestion importance[1-10]: 6

__

Why: Reasonable observation that hard-coding true for the null branch can be inconsistent with the field's declared nullability. This improves consistency between the schema and row literals, though the practical impact depends on downstream validation.

Low
General
Verify struct nullability for schema

Using TypeCreator.REQUIRED.struct(...) forces the outer struct to non-nullable, but
the individual field nullabilities in fieldTypes are preserved only for character
types via TypeCreator.of(f.getType().isNullable()). Ensure the struct's own
nullability matches the row type (typically non-nullable for Values is fine), but
verify this matches what downstream consumers expect for inline VALUES.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionFragmentConvertor.java [819]

+NamedStruct schema = NamedStruct.of(names, TypeCreator.REQUIRED.struct(fieldTypes));
 
-
Suggestion importance[1-10]: 1

__

Why: The suggestion only asks to verify, and the improved_code is identical to the existing_code, providing no actionable change.

Low
Suggestions up to commit f07f2eb
CategorySuggestion                                                                                                                                    Impact
Possible issue
Respect column nullability for typed null

When a null literal appears in a non-nullable column, emitting a nullable typed null
contradicts the schema and could fail conformance checks. Use the column's declared
nullability (from fieldTypes/cellType) for the typed null instead of hardcoding
true.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionFragmentConvertor.java [831-835]

 if (lit.isNull()) {
-        cells.add(ExpressionCreator.typedNull(TypeCreator.of(true).STRING));
+        cells.add(ExpressionCreator.typedNull(TypeCreator.of(cellType.isNullable()).STRING));
     } else {
         cells.add(ExpressionCreator.string(cellType.isNullable(), lit.getValueAs(String.class)));
     }
Suggestion importance[1-10]: 7

__

Why: Valid point: using TypeCreator.of(true).STRING for a typed null in a potentially non-nullable column could conflict with the schema nullability declared in fieldTypes. Aligning with cellType.isNullable() improves correctness of the row-to-schema conformance check.

Medium
General
Preserve row type nullability in schema

Wrapping the field list in TypeCreator.REQUIRED.struct(...) forces the outer struct
to be non-nullable but does not preserve the per-field nullability already encoded
in fieldTypes. Verify this matches how isthmus builds its schema struct; if the
outer struct should reflect the row type's nullability, use the appropriate
TypeCreator accordingly to avoid schema mismatches downstream.

sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionFragmentConvertor.java [822]

-NamedStruct schema = NamedStruct.of(names, TypeCreator.REQUIRED.struct(fieldTypes));
+NamedStruct schema = NamedStruct.of(names, TypeCreator.of(rowType.isNullable()).struct(fieldTypes));
Suggestion importance[1-10]: 3

__

Why: The suggestion is speculative ("Verify this matches...") and the outer struct nullability for a VirtualTableScan schema is typically REQUIRED; changing it may not be correct without deeper verification.

Low

@github-actions

Copy link
Copy Markdown
Contributor

✅ Gradle check result for da000e4: SUCCESS

@codecov

codecov Bot commented Jul 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.35%. Comparing base (03a4de3) to head (830d097).

Additional details and impacted files
@@             Coverage Diff              @@
##               main   #22554      +/-   ##
============================================
- Coverage     71.42%   71.35%   -0.08%     
+ Complexity    76786    76737      -49     
============================================
  Files          6148     6148              
  Lines        357980   357980              
  Branches      52177    52177              
============================================
- Hits         255689   255437     -252     
- Misses        81937    82184     +247     
- Partials      20354    20359       +5     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…version

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 <lingzhichu.clz@gmail.com>
@noCharger
noCharger force-pushed the fix/datafusion-virtualtable-char-normalization branch from da000e4 to 6ab8d92 Compare July 24, 2026 09:37
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 6ab8d92

@github-actions

Copy link
Copy Markdown
Contributor

✅ Gradle check result for 6ab8d92: SUCCESS

@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit f07f2eb

Signed-off-by: Louis Chu <lingzhichu.clz@gmail.com>
@noCharger
noCharger force-pushed the fix/datafusion-virtualtable-char-normalization branch from f07f2eb to 2e6d777 Compare July 30, 2026 15:30
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 2e6d777

Signed-off-by: Louis Chu <lingzhichu.clz@gmail.com>
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit efeee4b

…rted)

Signed-off-by: Louis Chu <lingzhichu.clz@gmail.com>
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 4e3d130

…via fields)

Signed-off-by: Louis Chu <lingzhichu.clz@gmail.com>
@noCharger
noCharger marked this pull request as ready for review July 30, 2026 16:35
@noCharger
noCharger requested a review from a team as a code owner July 30, 2026 16:35
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 0663a61

@noCharger noCharger self-assigned this Jul 30, 2026
@noCharger noCharger added the analytics-engine Issues and PR for the new Analytics engine label Jul 30, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 830d097

@github-actions

Copy link
Copy Markdown
Contributor

✅ Gradle check result for 830d097: SUCCESS

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

analytics-engine Issues and PR for the new Analytics engine

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant