Skip to content

CometLiteral accepts a collated string literal and serializes it as a plain string #6232

Description

@stantheman0128

What is the problem the feature request solves?

CometLiteral.getSupportLevel checks the literal's type through supportedDataType with the default allowAnyStringType = true, so its StringType case accepts every collation (QueryPlanSerde.scala:615, 624-625), and convert writes the value with setStringVal (literals.scala:98-99). So a Literal of type StringType(UTF8_LCASE) is accepted, and its collation is not carried into the native plan. Nothing records that it was dropped.

A cast of a literal is one way to get such a literal. CometCast.getSupportLevel returns Compatible() for a cast whose child is a Literal, unless a variant type is involved, without calling isSupported (CometCast.scala:90-99), so the collation guard proposed in #5302 would not see it either. convert then folds the cast with cast.eval() into a new literal of the target type (:111-112). CAST('abc' AS STRING COLLATE UTF8_LCASE) therefore turns into a collated literal that CometLiteral accepts. ConstantFolding normally removes a cast like that before Comet sees it, but CometSqlFileTestSuite excludes ConstantFolding for every fixture (CometSqlFileTestSuite.scala:92), so Comet's own harness can reach this path.

The answer is right today, since folding produces the same bytes Spark would. The problem is the one #4489 described for CometCast: the behavior is implicit and untested, so the next change to either serde can alter it without anyone noticing. I have not built a query that returns a wrong result through this path.

Describe the potential solution

Two options, and I don't have a strong preference:

  1. CometLiteral.getSupportLevel returns Unsupported when the literal's type has a non-default collation. Passing allowAnyStringType = false to supportedDataType would do it for a plain string literal, the way CometLocalTableScanExec already does (CometLocalTableScanExec.scala:146). CometLiteral also mixes in CometTypeShim, so hasNonDefaultStringCollation is in scope for nested types. The cost is that these literals, and the expressions around them, fall back to Spark.
  2. Keep accepting them, since the bytes match, and say so in a comment in CometLiteral.

Either way, a test under CometSqlFileTestSuite with CAST('abc' AS STRING COLLATE UTF8_LCASE) would pin the choice, since that harness is the one that can reach this path.

Additional context

Found while working on #5302 (#4489). Discussion: #5302 (review)

Line numbers are from main at 646ff181.

Activity

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

Metadata

Metadata

Assignees

Labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions