[multistage] clean up transform function type #11726
Conversation
Codecov Report
@@ Coverage Diff @@
## master #11726 +/- ##
=============================================
- Coverage 63.08% 14.43% -48.66%
+ Complexity 1117 201 -916
=============================================
Files 2342 2342
Lines 125797 125889 +92
Branches 19336 19360 +24
=============================================
- Hits 79358 18166 -61192
- Misses 40785 106190 +65405
+ Partials 5654 1533 -4121
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1509 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
walterddr
force-pushed
the
pr_clean_up_transform_scalar
branch
from
db184f3
to
0d9a304
Compare
walterddr
changed the title
| "arraylength", "arrayaverage", "arraymin", "arraymax", "arraysum", | ||
| "valuein", "mapvalue", "inidset", "lookup", "groovy", | ||
| "timeconvert", "datetimeconvert", "jsonextractscalar", "jsonextractkey", "clpdecode", | ||
| "arraylength", "arrayaverage", "arraymin", "arraymax", "arraysum", "clpdecode", "groovy", |
There was a problem hiding this comment.
other than the ignored list here, all other functions (including their alternative naming are registered in FunctionRegistry (e.g. has correspondent in ScalarFunction impl)
| NOT_EQUALS("not_equals"), | ||
| GREATER_THAN("greater_than"), | ||
| GREATER_THAN_OR_EQUAL("greater_than_or_equal"), | ||
| NOT_EQUALS("not_equals", "notEquals"), |
There was a problem hiding this comment.
Do we need these aliases for auto-generated functions? If so, why skipping LESS_THAN and NOT_IN?
There was a problem hiding this comment.
lol good catch. :-)
There was a problem hiding this comment.
technically we dont need any of these. but we already have ComparisonFunctions in scalar with these names so added lessThan but not notIn b/c we dont have that
There was a problem hiding this comment.
This is not the correct way to decide whether to add them though. Eventually we need scalar function for all of them, but that is orthogonal to this problem.
Does postgreSQL support these functions? If so, we need to add them, if not, we should not add them
There was a problem hiding this comment.
So 2 options. Either we allow underscore on both scalar and transform. Or we don't allow them in neither.
Problem is that transform function in v1 allows underscore so for backward compatibility we need to register them. It is not about syntax.
For syntax sake we should not allow underscore format all together.
There was a problem hiding this comment.
discussed offline. will remove these in separate PR as we should not allow these function explicitly being called. They are not consider SqlFunction but SqlOperators to begin with
due to the fact that we do allow these already in release 1.0, we will use a separate PR to deprecate them explicitly with backward-incompat flag
registration of the transform function is not complete with
_separate and camelCase naming.TODO: add tests to pinot-query-runtime (separate b/c there are bug fixes required)
CC @xiangfu0 @Jackie-Jiang