Repository navigation
Conversation
ggjh-159
left a comment
ggjh-159
left a comment
There was a problem hiding this comment.
-
This PR adds four mappings with no tests at all. Add SQL-level tests covering the three-valued truth tables of the four operators over a boolean column containing NULLs (NOT preserves NULL; the IS_* predicates never return NULL), nested forms such as NOT(c1 IS FALSE) and IS NOT TRUE(NOT c1), and constant / constant-foldable operands.
-
.github/workflows/flink.yml:90 still clones velox4j at the gluten-20260829 branch, whose pinned velox does not contain the
is_*functions these mappings resolve to, so the flink-test check cannot exercise them and would fail once tests are added. Point the clone at the velox4j branch carrying the velox ref bump for this change, and revert to gluten-20260829 once the velox-side functions land there.
| @@ -111,6 +111,11 @@ public class RexCallConverterFactory { | |||
| Map.entry("count_char", Arrays.asList(() -> new DefaultRexCallConverter("count_char"))), | |||
| Map.entry("EXTRACT", Arrays.asList(() -> new DefaultRexCallConverter("extract"))), | |||
| Map.entry("IS TRUE", Arrays.asList(() -> new IsTrueRexCallConverter())), | |||
There was a problem hiding this comment.
The IS TRUE entry maps to equalto(x, true), which returns NULL for a NULL input, while Flink x IS TRUE returns FALSE and never NULL (ScalarOperatorGens.scala:771-777, NEVER_NULL). Switch this entry to the flinksql is_true function once it is available, and include IS TRUE in the SQL-level tests, asserting FALSE for NULL input.
There was a problem hiding this comment.
Switched the IS TRUE entry to the flinksql is_true function (DefaultRexCallConverter), which returns FALSE and never NULL for a NULL input, and included IS TRUE in the SQL-level tests asserting FALSE for NULL — see the inline reply for details.
af394da to
d98b364
Compare
ef74649 to
d98b364
Compare
|
@ggjh-159 Thanks for the thorough review!
|
6cb6b99 to
df52202
Compare
- RexCallConverterFactory: add converter entries for NOT, IS FALSE, IS NOT TRUE, IS NOT FALSE (default name mapping to the flinksql- registered functions; NOT overwrites the baseline via the same name in the vector function tier) IS TRUE keeps the original IsTrueRexCallConverter.
20cbd9c to
4e50a85
Compare
|
@sunshone please update velox4j version and Flink.md |
… velox4j pin
- Switch the IS TRUE entry in RexCallConverterFactory from
IsTrueRexCallConverter (equalto(x, true), which returns NULL for a
NULL input) to DefaultRexCallConverter("is_true"): Flink x IS TRUE
returns FALSE for UNKNOWN and never NULL
(ScalarOperatorGens.scala:771-777, NEVER_NULL). IsTrueRexCallConverter
itself is baseline code and stays untouched.
- Add SQL-level tests (LogicalFunctionsTest, ScalarFunctionsTest
paradigm) over a BOOLEAN column containing NULLs: three-valued truth
tables for NOT/IS TRUE/IS FALSE/IS NOT TRUE/IS NOT FALSE (NOT
preserves NULL, the IS_* predicates never return NULL), nested forms
NOT(c1 IS FALSE) and (NOT(c1)) IS NOT TRUE (IS binds tighter than
NOT, so the parenthesized form is the one that applies IS NOT TRUE to
NOT c1), constant and constant-foldable operands including
CAST(NULL AS BOOLEAN) IS TRUE, and a WHERE-clause filter case.
- Point the flink CI velox4j clone at the sunshone fork's
flinksql-logical-expressions branch carrying the velox ref bump for
this change (fix-velox4j.patch applies cleanly on the pinned head),
reverting to bigo-sg gluten-20260829 once the velox-side functions
land there.
4e50a85 to
3717d81
Compare
What changes are proposed in this pull request?
Fixes #13457
Maps the Flink logical expressions to their velox flinksql implementations (companion velox PR bigo-sg/velox#90, velox4j ref bump bigo-sg/velox4j#71):
RexCallConverterFactory:NOT/IS FALSE/IS NOT TRUE/IS NOT FALSEentries — direct name mappings; these previously failed planning with "Function not supported".IS TRUE→DefaultRexCallConverter("is_true"): Flinkx IS TRUEreturns FALSE for UNKNOWN and never NULL (ScalarOperatorGens.scala:771-777, NEVER_NULL); the previousequalto(x, true)mapping returns NULL for a NULL input.IsTrueRexCallConverteritself is pre-existing baseline code and is left untouched..github/workflows/flink.yml): clone velox4j from the fork branch carrying the velox ref bump for this change (sunshone/velox4jflinksql-logical-expressions, pinned to the tip of that branch — the pin value lives in the workflow file and moves with the velox ref on each rebase), with a TODO recording the revert to bigo-sg/velox4j'sgluten-20260829branch (revert pin: see the TODO inflink.yml/Flink.md) once the velox-side flinksql functions land there;gluten-flink/docs/Flink.mdbuild instructions updated accordingly.The
is_*names exist only in the velox flinksql namespace (companion PR), so landing order / version pairing matters;notreuses the single prestosql NOT implementation registered under the flinksql prefix, moving dialect ownership of the runtime path.How was this patch tested?
LogicalFunctionsTestin gluten-flink/ut (theScalarFunctionsTestparadigm: bounded values table +runAndCheckchangelog comparison) over a BOOLEAN column containing NULLs — 8/8 green: three-valued truth tables for all five operators (NOT preserves NULL; the IS_* predicates never return NULL), nested formsNOT(c1 IS FALSE)and(NOT(c1)) IS NOT TRUE(IS binds tighter than NOT in SQL), constant and constant-foldable operands includingCAST(NULL AS BOOLEAN) IS TRUEasserting FALSE never NULL, and WHERE-clause filter cases.ScalarFunctionsTestregression 20/20.Source -> gluten-calc -> Sink, confirming the expressions are offloaded to velox rather than falling back.