Skip to content

Support a literal str argument in native lpad/rpad #7525

Open

Description

@tlfua

What is the problem the feature request solves?

CometStringLPad and CometStringRPad (spark/src/main/scala/org/apache/comet/serde/strings.scala) return Unsupported when the str argument is a Literal, so queries such as lpad('hi', len, 'xy') run through codegen dispatch instead of the native kernel.

The check was added in #2630 because the native read_side_padding kernel only accepted a string array, and a literal str failed at runtime. #7450 adds a scalar-string arm to the kernel: it handles a scalar str with a scalar or an array length, a NULL str, and a NULL length, without expanding the scalar. So the reason for the check no longer holds.

Raised by @coderfender in the #7450 review.

Describe the potential solution

  • Drop the expr.str.isInstanceOf[Literal] check (and PadReasons.literalStrReason) from both serdes. The pad argument still has to be a literal.
  • Update the lpad/rpad notes in docs/source/user-guide/latest/expressions.md, which say literal strings use codegen dispatch.
  • In string_lpad.sql and string_rpad.sql, switch the queries with a literal str and a literal pad, such as lpad('hi', len, 'xy') and lpad('hi', len), from expect_dispatch to expect_native. Queries whose pad is a column stay on codegen dispatch.
  • An all-literal call is constant-folded by Spark, so the tests need a column length, or ConstantFolding excluded.

Additional context

Depends on #7450.

Willingness to contribute

I would be willing to contribute a fix for this bug with guidance from the DataFusion community

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions