[jira] [Commented] (CALCITE-5479) FamilyOperandTypeChecker is not readily composable in sequences

["Gian Merlino (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/CALCITE-5479?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17690605#comment-17690605 ] 

Gian Merlino commented on CALCITE-5479:
---------------------------------------

I see. I've updated the linked PR with what I think is a better fix, based on this discussion.

Remarks on the new fix: I believe the problem is that FamilyOperandTypeChecker is used in two different styles.

First:
{code:java}
OperandTypes.sequence(OperandTypes.STRING, OperandTypes.NUMERIC)
{code}
In this style, two FamilyOperandTypeCheckers, STRING and NUMERIC, are wrapped in a CompositeOperandTypeChecker. For this to work with the current implementation of FamilyOperandTypeChecker, {{iFormalOperand}} must be zero. I suppose this is why CompositeOperandTypeChecker has some logic that sets it to zero specifically for FamilyOperandTypeChecker.

Second:
{code:java}
OperandTypes.family(SqlTypeFamily.STRING, SqlTypeFamily.NUMERIC)
{code}
In this style, there is a single FamilyOperandTypeChecker which behaves the same as the composite sequence above. For this to work, the FamilyOperandTypeCheckers do need to be aware of the operand positions.

These two styles are difficult to reconcile, hence the cracks that need papering.

The fix in the updated PR is to:
 # Have {{CompositeOperandTypeChecker}} send down {{ord.i}} for {{iFormalOperand}} always.
 # Have {{FamilyOperandTypeChecker#checkSingleOperandType}}, the method from the {{SqlSingleOperandTypeChecker}} interface, ignore {{iFormalOperand}}. So, when the checker is being used as a {{SqlSingleOperandTypeChecker}} — as it is inside a sequence — it ignores operand position and always uses the zeroth family. I cannot think of a valid reason for it to do anything else, given how it is typically composed into sequences.
 # {{FamilyOperandTypeChecker}} continues being aware of operand position when used as a {{SqlOperandTypeChecker}} via {{checkOperandTypes}}.

> FamilyOperandTypeChecker is not readily composable in sequences
> ---------------------------------------------------------------
>
>                 Key: CALCITE-5479
>                 URL: https://issues.apache.org/jira/browse/CALCITE-5479
>             Project: Calcite
>          Issue Type: Bug
>            Reporter: Gian Merlino
>            Priority: Major
>              Labels: pull-request-available
>          Time Spent: 20m
>  Remaining Estimate: 0h
>
> Handling for {{OperandTypes.sequence}} changed in [33f4ab40bbee26e06209061c35a422f2f1e05371|https://github.com/apache/calcite/commit/33f4ab40bbee26e06209061c35a422f2f1e05371#diff-b0b8d58a792b8e60b9e97717912aecfc6695536f5026ac4d5231d14e34b91566L303-R316] such that {{iFormalOperand}} passed to subcheckers is no longer always zero, but is instead:
> - Zero if the subchecker is {{FamilyOperandTypeChecker}}.
> - Otherwise, the operand number in the overall sequence.
> It causes problems for the way we're using sequence checkers in Druid, since we don't always use {{FamilyOperandTypeChecker}}, but we _do_ assume the old behavior: that {{iFormalOperand}} is always zero, and therefore we can put any checker into the sequence without it being "aware" that it is in a sequence.
> I marked this as a bug in case this change was made accidentally. If it was made for a reason, please let me know. Thanks.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)