Skip to content

feat: Implement map-to-string casting - #4647

Open
getChan wants to merge 4 commits into
apache:mainfrom
getChan:fix-map-cast-string
Open

feat: Implement map-to-string casting#4647
getChan wants to merge 4 commits into
apache:mainfrom
getChan:fix-map-cast-string

Conversation

@getChan

@getChan getChan commented Jun 13, 2026

Copy link
Copy Markdown

Which issue does this PR close?

Closes #4631.

Rationale for this change

This PR implements native map-to-string casting in Comet to match Spark formatting ({k -> v, ...}).
As part of completing this feature, it also covers the empty-map case (cast(map() as string)) where Catalyst infers Map<NullType, NullType>.

What changes are included in this PR?

  • Implement native map-to-string rendering in native/spark-expr.
  • Wire Spark-side cast support for MapType -> StringType in CometCast.
  • Include empty map support by handling NullType -> StringType in cast support checks.
  • Extend codegen NullType handling needed by Map<NullType, NullType>:
    • type admission for NullType
    • output path (NullVector, null-only write/getter handling)
    • input nested read/copy path (NullType getter/copy handling).
  • Update SQL test expectation so cast(map() as string) runs natively instead of fallback.

How are these changes tested?

  • PROFILES='-Pspark-4.0' make format
  • ./mvnw -q test -DwildcardSuites=CometSqlFileTestSuite -Dsuites='org.apache.comet.CometSqlFileTestSuite cast_complex_types_to_string' -Dtest=none

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@andygrove andygrove changed the title Implement map-to-string casting feat: Implement map-to-string casting Jun 13, 2026
query expect_fallback(Cast from MapType)
-- Empty map: still falls back because planning sees `map()` as `Map<NullType, NullType>`,
-- which reaches the existing NullType -> StringType cast fallback.
query expect_fallback(Cast from NullType to StringType is not supported)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if this is the intended behavior. How should we handle the empty map(Map<NullType, NullType) case?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

technically we can cast it, we have everything for it.
Perhaps it would be enough to add NullType to list of supported types?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@comphead Thanks for review. I updated the code to support empty maps. Please refer to the PR description for details.

getChan and others added 2 commits June 21, 2026 23:37
Allow map nulltype key value to execute natively for cast map to string by enabling nulltype support in cast and codegen input output paths.

Update SQL expectation for empty map from fallback to native query.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@@ -672,6 +673,8 @@ private[codegen] object CometBatchKernelCodegenInput {
if (elementNullable) " if (isNullAt(i)) return null;\n"

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is intentional. NullType has no typed getter to override (there is no getNull API), so we emit no scalar getter here. Null semantics are handled via isNullAt (always true for NullVector) and generic get(..., NullType) dispatch, which returns null.

@github-actions

Copy link
Copy Markdown

Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.

@github-actions github-actions Bot added the Stale label Aug 21, 2026
@andygrove

Copy link
Copy Markdown
Member

Note on this review: this was generated by an LLM (Claude Code) at my request while I worked through a review backlog. I have not verified the individual findings myself. Please treat everything below as suggestions to evaluate rather than as authoritative review feedback, and push back on anything that is wrong or already handled.

The rendering matches Spark's {k -> v, ...} and reusing cast_array for the keys and values so nested types compose is the right structure. Flipping a good number of expect_fallback fixtures back to plain query is a satisfying diff to read.

Four things.

Does this honor spark.sql.legacy.castComplexTypesToString.enabled?

cast_map_to_string hardcodes {, }, and " -> ". When that legacy config is on, Spark renders maps with [ and ] and omits NULL elements instead of writing the literal null. CometCast.scala already has a comment about only supporting the non-legacy behavior for structs and arrays, so presumably the same guard needs to cover MapType -> StringType.

Is the map case gated by that check today? If not, a user with the legacy config gets a silently different string. If it is gated, a test with the config on asserting a fallback would pin it.

Hidden entries outside a slice or under a null row

The function casts the entire keys() and values() children up front, then indexes them with the row offsets. For a sliced MapArray, or one with null rows, the children contain entries that the visible rows never reference. Under a throwing eval mode, an ANSI cast failure on one of those hidden values would fail the query even though Spark never evaluates it.

This is the same class of problem #5457 is fixing for nested casts generally. Is it reachable here? If the fix lands in prepare_nested_cast_input at the top of cast_array, it may cover this for free, but it would be worth confirming rather than assuming.

NullType is now admitted to the codegen dispatcher globally

def isSupportedDataType(dt: DataType): Boolean = dt match {
  case NullType => true

That is in CometBatchKernelCodegen, so it admits NullType for every dispatched expression, not just this cast. The supporting changes generate the literal null from elementGetterCall and emitSpecializedGetterExpr, which compiles where an Object is expected and does not where a primitive is.

Is there an expression where a NullType child would land in a primitive context and produce uncompilable Java? A test that dispatches some other expression over a NullType column would be worth having, since the failure mode is a Janino compile error at runtime rather than something CI catches.

The propagation test was inverted

cast MapType propagates Unsupported from nested value cast existed specifically to prove that an unsupported nested cast is not silently swallowed by the outer Map arm. It has been rewritten to assert Compatible for the now-supported case.

That is correct, but it means the original property is no longer covered. Could you add back a case with a genuinely unsupported nested type, for example Map<Int, Binary> to string, asserting that the Unsupported still propagates through the outer arm? Otherwise a future change that swallows the inner result would go unnoticed.

@github-actions github-actions Bot removed the Stale label Aug 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

chore: Improve cast MapType to String

3 participants