[jira] [Comment Edited] (PHOENIX-3355) Register Phoenix built-in functions as Calcite functions
[ https://issues.apache.org/jira/browse/PHOENIX-3355?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15610029#comment-15610029 ] Eric Lomore edited comment on PHOENIX-3355 at 10/26/16 11:35 PM: - [~maryannxue] thanks for all the help. I've attached a new patch, while it's not fully there I think this is closer to what we want. Few things I'm aware of and considering options for: - We could opt to modify initBuiltinFunctionMap() to create and register the builtin functions, essentially pushing all the work there away from PhoenixSchema.java. No functional difference. - I haven't modified getDataType(). For the most part this is OK, most functions have accessible return types, and the vast majority of functions get registered. Exhaustive list of those having issues: {code}getDataType() error: ARRAY_CAT getDataType() error: FLOOR getDataType() error: ARRAY_APPEND getDataType() error: CEIL getDataType() error: REGEXP_REPLACE getDataType() error: SET_BYTE getDataType() error: ARRAY_FILL getDataType() error: REGEXP_SUBSTR getDataType() error: ARRAY_ELEM getDataType() error: NOW getDataType() error: ROUND getDataType() error: TRUNC getDataType() error: REGEXP_SPLIT getDataType() error: ARRAY_PREPEND getDataType() error: SET_BIT{code} Many of these are translated in various parts of CalciteUtils (Floor, Ceil), should these also be added to the TRANSLATED map? The rest that are throwing errors I have not been able to resolve the issue of output being dependent on input. - I attempted making {{TRANSLATED_BUILT_IN_FUNCTIONS}} a map, and creating a utility function to create instances of each function but since each function requires different parameters it may not be worth it - the code would be more breakable. {code}static { TRANSLATED_BUILT_IN_FUNCTIONS.put(SqrtFunction.NAME, SqrtFunction.class); TRANSLATED_BUILT_IN_FUNCTIONS.put(PowerFunction.NAME, PowerFunction.class); TRANSLATED_BUILT_IN_FUNCTIONS.put(LnFunction.NAME, LnFunction.class); TRANSLATED_BUILT_IN_FUNCTIONS.put(ExpFunction.NAME, ExpFunction.class); TRANSLATED_BUILT_IN_FUNCTIONS.put(AbsFunction.NAME, AbsFunction.class); TRANSLATED_BUILT_IN_FUNCTIONS.put(CurrentDateFunction.NAME, CurrentDateFunction.class); TRANSLATED_BUILT_IN_FUNCTIONS.put(CurrentTimeFunction.NAME, CurrentTimeFunction.class); TRANSLATED_BUILT_IN_FUNCTIONS.put(LowerFunction.NAME, LowerFunction.class); TRANSLATED_BUILT_IN_FUNCTIONS.put(UpperFunction.NAME, UpperFunction.class); TRANSLATED_BUILT_IN_FUNCTIONS.put(CoalesceFunction.NAME, CoalesceFunction.class); }{code} {code}} else if (TRANSLATED_BUILT_IN_FUNCTIONS.get(op.getName()) != null){ try { return TRANSLATED_BUILT_IN_FUNCTIONS.get(op.getName()).getConstructor() .newInstance(children); } catch(Exception e) { throw new RuntimeException(e);}{code} - Aggregate function TODOs noted in the patch. was (Author: lomoree): [~maryannxue] thanks for all the help. I've attached a new patch, while it's not fully there I think this is closer to what we want. Few things I'm aware of and considering options for: - We could opt to modify initBuiltinFunctionMap() to create and register the builtin functions, essentially pushing all the work there away from PhoenixSchema.java. No functional difference. - I haven't modified getDataType(). For the most part this is OK, most functions have accessible return types, and the vast majority of functions get registered. Exhaustive list of those having issues: {code}getDataType() error: ARRAY_CAT getDataType() error: FLOOR getDataType() error: ARRAY_APPEND getDataType() error: CEIL getDataType() error: REGEXP_REPLACE getDataType() error: SET_BYTE getDataType() error: ARRAY_FILL getDataType() error: REGEXP_SUBSTR getDataType() error: ARRAY_ELEM getDataType() error: NOW getDataType() error: ROUND getDataType() error: TRUNC getDataType() error: REGEXP_SPLIT getDataType() error: ARRAY_PREPEND getDataType() error: SET_BIT{code} Many of these are translated in CalciteUtils (Floor, Ceil), should these also be added to the TRANSLATED map? Those that aren't have input child expressions that aren't accessible (not coincidental that many are array functions) - I attempted making {{TRANSLATED_BUILT_IN_FUNCTIONS}} a map, and creating a utility function to create instances of each function but since each functions needs a specific constructor it's likely not probable. {code}} else if (TRANSLATED_BUILT_IN_FUNCTIONS.get(op.getName()) != null){ try { return TRANSLATED_BUILT_IN_FUNCTIONS.get(op.getName()).getConstructor() .newInstance(children);
[jira] [Comment Edited] (PHOENIX-3355) Register Phoenix built-in functions as Calcite functions
[ https://issues.apache.org/jira/browse/PHOENIX-3355?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15606368#comment-15606368 ] Maryann Xue edited comment on PHOENIX-3355 at 10/25/16 8:26 PM: [~rajeshbabu], Take a closer look at the plan with default values. Calcite interpret the default value as a special sql operator "DEFAULT()", and we did not handle that operator in our translation logic yet. So try adding the following to your CalciteUtils.java, and it'll work fine. {code} @@ -734,6 +734,18 @@ public class CalciteUtils { } } }); +EXPRESSION_MAP.put(SqlKind.DEFAULT, new ExpressionFactory() { +@SuppressWarnings("rawtypes") +@Override +public Expression newExpression(RexNode node, PhoenixRelImplementor implementor) { +PDataType targetType = relDataTypeToPDataType(node.getType()); +try { +return LiteralExpression.newConstant(null, targetType); +} catch (SQLException e) { +throw new RuntimeException(e); +} +} + }); EXPRESSION_MAP.put(SqlKind.OTHER_FUNCTION, new ExpressionFactory() { @Override public Expression newExpression(RexNode node, {code} was (Author: maryannxue): [~rajeshbabu], Take a closer look at the plan with default values. Calcite interpret the default value as a special sql operator "DEFAULT()", and we did not handle that operator in our translation logic yet. So try adding the following to your CalciteUtils.java, and it'll work fine. {code} @@ -734,6 +734,18 @@ public class CalciteUtils { } } }); + EXPRESSION_MAP.put(SqlKind.DEFAULT, new ExpressionFactory() { +@SuppressWarnings("rawtypes") +@Override +public Expression newExpression(RexNode node, PhoenixRelImplementor implementor) { +PDataType targetType = relDataTypeToPDataType(node.getType()); +try { +return LiteralExpression.newConstant(null, targetType); +} catch (SQLException e) { +throw new RuntimeException(e); +} +} + }); EXPRESSION_MAP.put(SqlKind.OTHER_FUNCTION, new ExpressionFactory() { @Override public Expression newExpression(RexNode node, {code} > Register Phoenix built-in functions as Calcite functions > > > Key: PHOENIX-3355 > URL: https://issues.apache.org/jira/browse/PHOENIX-3355 > Project: Phoenix > Issue Type: Bug >Reporter: James Taylor >Assignee: Eric Lomore > Labels: calcite > Attachments: PHOENIX-3355.wip > > > We should register all Phoenix built-in functions that don't exist in Calcite. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Comment Edited] (PHOENIX-3355) Register Phoenix built-in functions as Calcite functions
[ https://issues.apache.org/jira/browse/PHOENIX-3355?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15603603#comment-15603603 ] Maryann Xue edited comment on PHOENIX-3355 at 10/24/16 11:45 PM: - I just suggested using a temporary map and iterate the BUILT_IN_FUNCTION_MAP just once. The temporary map will be somewhat mapping a function name to all its available signatures. So if we going to register COALESCE as an UDF, we'll have to register all its overloads. Another option is to skip registering COALESCE since calcite has support for it already. If you take a look at line 738 of CalciteUtils.java, you can see there are a few built-in functions already handled by this translation logic {{EXPRESSION_MAP.put(SqlKind.OTHER_FUNCTION, new ExpressionFactory()}}. For each of our built-in function that's also supported in Calcite, we'll have to decide whether to use the Calcite support or override with our UDF registration. Basically we are free to choose, if it IS overridable, and if the signatures are identical. But let's just have a static list, say, in CalciteUtils, "TRANSLATED_BUILT_IN_FUNCTIONS", so that your UDF registration logic can skip them. And maybe you can COALESCE in this list. was (Author: maryannxue): I just suggested using a temporary map and iterate the BUILT_IN_FUNCTION_MAP just once. The temporary map will be somewhat mapping a function name to all its available signatures. So if we going to register COALESCE as an UDF, we'll have to register all its overloads. Another option is to skip registering COALESCE since calcite has support for it already. If you take a line 738 of CalciteUtils.java, you can see there are a few built-in functions already handled by this translation logic {{EXPRESSION_MAP.put(SqlKind.OTHER_FUNCTION, new ExpressionFactory()}}. For each of our built-in function that's also supported in Calcite, we'll have to decide whether to use the Calcite support or override with our UDF registration. Basically we are free to choose, if it IS overridable, and if the signatures are identical. But let's just have a static list, say, in CalciteUtils, "TRANSLATED_BUILT_IN_FUNCTIONS", so that your UDF registration logic can skip them. And maybe you can COALESCE in this list. > Register Phoenix built-in functions as Calcite functions > > > Key: PHOENIX-3355 > URL: https://issues.apache.org/jira/browse/PHOENIX-3355 > Project: Phoenix > Issue Type: Bug >Reporter: James Taylor >Assignee: Eric Lomore > Labels: calcite > Attachments: PHOENIX-3355.wip > > > We should register all Phoenix built-in functions that don't exist in Calcite. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Comment Edited] (PHOENIX-3355) Register Phoenix built-in functions as Calcite functions
[ https://issues.apache.org/jira/browse/PHOENIX-3355?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15562837#comment-15562837 ] Eric Lomore edited comment on PHOENIX-3355 at 10/10/16 5:09 PM: Thanks [~maryannxue], the UDF approach makes sense. {{"CREATE VIEW v as SELECT * FROM t WHERE k1=TO_DATE('2016-06-06')";}} executes successfully was (Author: lomoree): {{"CREATE VIEW v as SELECT * FROM t WHERE k1=TO_DATE('2016-06-06')";}} executes successfully > Register Phoenix built-in functions as Calcite functions > > > Key: PHOENIX-3355 > URL: https://issues.apache.org/jira/browse/PHOENIX-3355 > Project: Phoenix > Issue Type: Bug >Reporter: James Taylor >Assignee: Eric Lomore > Labels: calcite > > We should register all Phoenix built-in functions that don't exist in Calcite. -- This message was sent by Atlassian JIRA (v6.3.4#6332)