Sounds like a great suggestion, Becket. +1. Agree with cleaning up the APIs
and making it consistent in all the pushdown APIs.
Your suggested approach seems fine to me, unless anyone else has any other
concerns. Just have couple of clarifying questions:
1. Do you think we should standardize the APIs across all the pushdown
supports like SupportsPartitionPushdown, SupportsDynamicFiltering etc in
the end state?
The current proposal works if we do not want to migrate
> SupportsFilterPushdown to also use NestedFieldReferenceExpression in the
> long term.
>
Did you mean *FieldReferenceExpression* instead of
*NestedFieldReferenceExpression*?
2. Extend the FieldReferenceExpression to support nested fields.
> - Change the index field type from int to int[].
- Add a new method int[] getFieldIndexArray().
> - Deprecate the int getFieldIndex() method, the code will be removed in
> the next major version bump.
I assume getFieldIndex would return fieldIndexArray[0], right?
Thanks
Venkat
On Fri, Aug 18, 2023 at 4:47 PM Becket Qin <becket....@gmail.com> wrote:
> Thanks for the proposal, Venkata.
>
> The current proposal works if we do not want to migrate
> SupportsFilterPushdown to also use NestedFieldReferenceExpression in the
> long term.
>
Did you mean *FieldReferenceExpression* instead of
*NestedFieldReferenceExpression*?
>
> Otherwise, the alternative solution briefly mentioned in the rejected
> alternatives would be the following:
> Phase 1:
> 1. Introduce a supportsNestedFilters() method to the SupportsFilterPushdown
> interface. (same as current proposal).
> 2. Extend the FieldReferenceExpression to support nested fields.
> - Change the index field type from int to int[].
- Add a new method int[] getFieldIndexArray().
> - Deprecate the int getFieldIndex() method, the code will be removed in
> the next major version bump.
3. In the SupportsProjectionPushDown interface
> - add a new method applyProjection(List<FieldReferenceExpression>,
> DataType), with default implementation invoking applyProjection(int[][],
> DataType)
> - deprecate the current applyProjection(int[][], DataType) method
>
> Phase 2 (in the next major version bump)
> 1. remove the deprecated methods.
>
> Phase 3 (optional)
> 1. deprecate and remove the supportsNestedFilters() /
> supportsNestedProjection() methods from the SupportsFilterPushDown /
> SupportsProjectionPushDown interfaces.
>
> Personally I prefer this alternative. It takes longer to finish the work,
> but the API eventually becomes clean and consistent. But I can live with
> the current proposal.
>
> Thanks,
>
> Jiangjie (Becket) Qin
>
> On Sat, Aug 19, 2023 at 12:09 AM Venkatakrishnan Sowrirajan <
> vsowr...@asu.edu> wrote:
>
> > Gentle ping for reviews/feedback.
> >
> > On Tue, Aug 15, 2023, 5:37 PM Venkatakrishnan Sowrirajan <
> vsowr...@asu.edu
> > >
> > wrote:
> >
> > > Hi All,
> > >
> > > I am opening this thread to discuss FLIP-356: Support Nested Fields
> > > Filter Pushdown. The FLIP can be found at
> > >
> >
> https://urldefense.com/v3/__https://cwiki.apache.org/confluence/display/FLINK/FLIP-356*3A*Support*Nested*Fields*Filter*Pushdown__;JSsrKysr!!IKRxdwAv5BmarQ!clxXJwshKpn559SAkQiieqgGe0ZduXCzUKCmYLtFIbQLmrmEEgdmuEIM8ZM1M3O_uGqOploU4ailqGpukAg$
> > >
> > > This FLIP adds support for pushing down nested fields filters to the
> > > underlying TableSource. In our data lake, we find a lot of datasets
> have
> > > nested fields and also user queries with filters defined on the nested
> > > fields. This would drastically improve the performance for those sets
> of
> > > queries.
> > >
> > > Appreciate any comments or feedback you may have on this proposal.
> > >
> > > Regards
> > > Venkata krishnan
> > >
> >
>
