HIVE-24599 :Add support vectorized two parameter trim functions - #6616
HIVE-24599 :Add support vectorized two parameter trim functions#6616Manya0407 wants to merge 3 commits into
Conversation
| /** | ||
| * Vectorized LTRIM with a scalar string and a trim-characters column. | ||
| */ | ||
| public class StringLTrimScalarCol extends VectorExpression { |
There was a problem hiding this comment.
Why is this class extending VectorExpression? There is StringUnaryUDF and StringUnaryUDFDirect to handle string functions that takes one string column argument.
There was a problem hiding this comment.
StringUnaryUDFDirect is for unary expressions with a single column input (e.g. StringTrimCol, StringTrimColScalar). The *ScalarCol classes are binary SCALAR + COLUMN expressions: a fixed scalar string plus a per-row trim-characters column. That operand pattern doesn’t match the unary base class API.
For SCALAR + COLUMN, the existing precedent in Hive is StringScalarConcatStringGroupCol, which also extends VectorExpression directly and implements its own evaluate() loop. We followed the same approach for StringTrimScalarCol, StringLTrimScalarCol, and StringRTrimScalarCol.
COL + SCALAR (trim(col, 'xy')) already uses StringTrimColScalarBase → StringUnaryUDFDirect. COL + COLUMN uses NullUtil.propagateNullsColCol in the ColCol classes.
I can extract a shared base for the three *ScalarCol classes if you’d prefer less duplication in evaluate(), but extending StringUnaryUDFDirect wouldn’t be the right fit for this operand pattern.
There was a problem hiding this comment.
That operand pattern doesn’t match the unary base class API.
Do you mean that the implementation for processing the new trim function calls with SCALAR + COLUMN expression arguments would be different from the one in StringUnaryUDFDirect or StringUnaryUDF?
If that is the case, it still makes sense to extract the duplicated code. So please go ahead.
There was a problem hiding this comment.
Yes — the SCALAR + COLUMN path is a different operand pattern from what StringUnaryUDF / StringUnaryUDFDirect handle.
I've extracted the shared logic into StringTrimScalarColBase. StringTrimScalarCol, StringLTrimScalarCol, and StringRTrimScalarCol now only implement func() (calling trimBoth / trimLeft / trimRight), mirroring the StringTrimColColBase pattern.
asf-ci-hive
added
tests pending
tests passed
and removed
tests passed
tests pending
labels
|
3 participants
What changes were proposed in this pull request?
This PR adds native vectorized support for the remaining two-parameter trim, ltrim, and rtrim operand patterns: column+column (e.g. trim(col0, col1)) and scalar+column (e.g. trim('str', col1)). It introduces six new vector expression classes, registers them on the existing Generic UDFs, and reuses shared trim logic in StringTrimColScalarBase. Unit and q-tests were added/extended to verify class selection, correct results, and NULL handling.
Why are the changes needed?
Vectorized support already existed for:
a.)one-argument trim: trim(col0) → StringTrimCol b.)column + scalar: trim(col0, 'xy') → StringTrimColScalar
For trim(col0, col1) and trim('str', col1) (and the ltrim/rtrim equivalents), Hive fell back to VectorUDFAdaptor in vectorized plans, which is slower than native vector expressions. This PR completes vectorization for all two-parameter trim operand patterns.
Does this PR introduce any user-facing change?
No
How was this patch tested?
1.Unit test: mvn -pl ql test -Dtest=TestVectorizationContext#testTwoParameterTrimVectorExpressions — verifies descriptor → vector class mapping for all six new classes.
2.Integration tests: TestMiniLlapLocalCliDriver with udf_trim_vector.q, udf_ltrim_vector.q, and udf_rtrim_vector.q — verifies native vector class selection in explain output, correct trim results, and NULL semantics.