[
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)
