Re: [DISCUSS] FLIP-356: Support Nested Fields Filter Pushdown

Thanks Martijn. Regards Venkata krishnan On Fri, Sep 22, 2023 at 1:51 AM Martijn Visser wrote: > Sure thing, I've made the necessary changes. Thnx for clarifying > > On Thu, Sep 21, 2023 at 8:24 PM Venkatakrishnan Sowrirajan > wrote: > > > > Got it, Martijn. > > > > Unfortunately, I don't hav

Re: [DISCUSS] FLIP-356: Support Nested Fields Filter Pushdown

Sure thing, I've made the necessary changes. Thnx for clarifying On Thu, Sep 21, 2023 at 8:24 PM Venkatakrishnan Sowrirajan wrote: > > Got it, Martijn. > > Unfortunately, I don't have edit access to the already created JIRA - > FLINK-20767 . If y

Re: [DISCUSS] FLIP-356: Support Nested Fields Filter Pushdown

Got it, Martijn. Unfortunately, I don't have edit access to the already created JIRA - FLINK-20767 . If you can remove the task from the EPIC FLINK-16987 FLIP-95: Add new table source and sink interfaces

Re: [DISCUSS] FLIP-356: Support Nested Fields Filter Pushdown

Hi Venkatakrishnan, The reason why I thought it's abandoned because the Jira ticket is part of the umbrella ticket for FLIP-95. Let's move the Jira ticket to its own dedicated task instead of nested to a FLIP-95 ticket. Thanks, Martijn On Wed, Sep 20, 2023 at 4:34 PM Becket Qin wrote: > > Hi M

Re: [DISCUSS] FLIP-356: Support Nested Fields Filter Pushdown

Hi Martijn, This FLIP has passed voting[1]. It is a modification on top of the FLIP-95 interface. Thanks, Jiangjie (Becket) Qin [1] https://lists.apache.org/thread/hysv9y1f48gtpr5vx3x40wtjb6cp9ky6 On Wed, Sep 20, 2023 at 9:29 PM Martijn Visser wrote: > For clarity purposes, this FLIP is bein

Re: [DISCUSS] FLIP-356: Support Nested Fields Filter Pushdown

No, this is not abandoned. This is accepted with enough binding votes. I didn't get why you think that this FLIP is abandoned. Could you please clarify? On Wed, Sep 20, 2023, 6:11 AM Martijn Visser wrote: > For clarity purposes, this FLIP is being abandoned because it was part > of FLIP-95? > >

Re: [DISCUSS] FLIP-356: Support Nested Fields Filter Pushdown

For clarity purposes, this FLIP is being abandoned because it was part of FLIP-95? On Thu, Sep 7, 2023 at 3:01 AM Venkatakrishnan Sowrirajan wrote: > > Hi everyone, > > Posted a PR (https://github.com/apache/flink/pull/23313) to add nested > fields filter pushdown. Please review. Thanks. > > Rega

Re: [DISCUSS] FLIP-356: Support Nested Fields Filter Pushdown

Hi everyone, Posted a PR (https://github.com/apache/flink/pull/23313) to add nested fields filter pushdown. Please review. Thanks. Regards Venkata krishnan On Tue, Sep 5, 2023 at 10:04 PM Venkatakrishnan Sowrirajan wrote: > Based on an offline discussion with Becket Qin, I added *fieldIndices

Re: [DISCUSS] FLIP-356: Support Nested Fields Filter Pushdown

Based on an offline discussion with Becket Qin, I added *fieldIndices *back which is the field index of the nested field at every level to the *NestedFieldReferenceExpression *in FLIP-356 *. *2 rea

Re: [DISCUSS] FLIP-356: Support Nested Fields Filter Pushdown

Hi Venkata, > Also I made minor changes to the *NestedFieldReferenceExpression, *instead > of *fieldIndexArray* we can just do away with *fieldNames *array that > includes fieldName at every level for the nested field. I don't think keeping only the field names array would work. At the end of t

Re: [DISCUSS] FLIP-356: Support Nested Fields Filter Pushdown

Gentle ping on the vote for FLIP-356: Support Nested fields filter pushdown . Regards Venkata krishnan On Tue, Aug 29, 2023 at 9:18 PM Venkatakrishnan Sowrirajan wrote: > Sure, will reference this discussion to resume where we st

Re: [DISCUSS] FLIP-356: Support Nested Fields Filter Pushdown

Sure, will reference this discussion to resume where we started as part of the flip to refactor SupportsProjectionPushDown. On Tue, Aug 29, 2023, 7:22 PM Jark Wu wrote: > I'm fine with this. `ReferenceExpression` and `SupportsProjectionPushDown` > can be another FLIP. However, could you summariz

Re: [DISCUSS] FLIP-356: Support Nested Fields Filter Pushdown

I'm fine with this. `ReferenceExpression` and `SupportsProjectionPushDown` can be another FLIP. However, could you summarize the design of this part in the future part of the FLIP? This can be easier to get started with in the future. Best, Jark On Wed, 30 Aug 2023 at 02:45, Venkatakrishnan Sow

Re: [DISCUSS] FLIP-356: Support Nested Fields Filter Pushdown

Thanks Jark. Sounds good. One more thing, earlier in my summary I mentioned, Introduce a new *ReferenceExpression* (or *BaseReferenceExpression*) > abstract class which will be extended by both *FieldReferenceExpression* > and *NestedFieldReferenceExpression* (to be introduced as part of this >

Re: [DISCUSS] FLIP-356: Support Nested Fields Filter Pushdown

Hi Venkata, Your summary looks good to me. +1 to start a vote. I think we don't need "inputIndex" in NestedFieldReferenceExpression. Actually, I think it is also not needed in FieldReferenceExpression, and we should try to remove it (another topic). The RexInputRef in Calcite also doesn't require

Re: [DISCUSS] FLIP-356: Support Nested Fields Filter Pushdown

Hi Jinsong, Thanks for your comments. What is inputIndex in NestedFieldReferenceExpression? I haven't looked at it before. Do you mean, given that it is now only used to push filters it won't be subsequently used in further planning/optimization and therefore it is not required at this time? S

Re: [DISCUSS] FLIP-356: Support Nested Fields Filter Pushdown

So if NestedFieldReferenceExpression doesn't need inputIndex, is there a need to introduce a base class `ReferenceExpression`? Best, Jingsong On Mon, Aug 28, 2023 at 2:09 PM Jingsong Li wrote: > > Hi thanks all for your discussion. > > What is inputIndex in NestedFieldReferenceExpression? > > I

Re: [DISCUSS] FLIP-356: Support Nested Fields Filter Pushdown

Hi thanks all for your discussion. What is inputIndex in NestedFieldReferenceExpression? I know inputIndex has special usage in FieldReferenceExpression, but it is only for Join operators, and it is only for SQL optimization. It looks like there is no requirement for Nested. Best, Jingsong On M

Re: [DISCUSS] FLIP-356: Support Nested Fields Filter Pushdown

Thanks for all the feedback and discussion everyone. Looks like we have reached a consensus here. Just to summarize: 1. Introduce a new *ReferenceExpression* (or *BaseReferenceExpression*) abstract class which will be extended by both *FieldReferenceExpression* and *NestedFieldReferenceExpression

Re: [DISCUSS] FLIP-356: Support Nested Fields Filter Pushdown

Hi Jark, How about having a separate NestedFieldReferenceExpression, and > abstracting a common base class "ReferenceExpression" for > NestedFieldReferenceExpression and FieldReferenceExpression? This makes > unifying expressions in > "SupportsProjectionPushdown#applyProjections(List > ...)" > pos

Re: [DISCUSS] FLIP-356: Support Nested Fields Filter Pushdown

Hi, Venkat, Thanks for the FLIP, it sounds good to support nested fields filter pushdown. Based on the design of flip and the above options, I would like to make a few suggestions: 1. At present, introducing NestedFieldReferenceExpression looks like a better solution, which can fully meet our re

Re: [DISCUSS] FLIP-356: Support Nested Fields Filter Pushdown

To keep it backwards compatible, introduce another API *applyAggregates *with *List *when nested field support is added and deprecate the current API. This will by default throw an exception. In flink planner, *applyAggregates *with nested fields and if it throws exception then *applyAggregates* wi

Re: [DISCUSS] FLIP-356: Support Nested Fields Filter Pushdown

Jark, How about having a separate NestedFieldReferenceExpression, and > abstracting a common base class "ReferenceExpression" for > NestedFieldReferenceExpression and FieldReferenceExpression? This makes > unifying expressions in > "SupportsProjectionPushdown#applyProjections(List > ...)" > possib

Re: [DISCUSS] FLIP-356: Support Nested Fields Filter Pushdown

Hi Becket, I think it is the second case, that a FieldReferenceExpression is constructed by the framework and passed to the connector (interfaces listed by Venkata[1] and Catalog#listPartitionsByFilter). Besides, understanding the nested field is optional for users/connectors (just treat it as an

Re: [DISCUSS] FLIP-356: Support Nested Fields Filter Pushdown

Becket and Jark, Deprecate all the other > methods except tryApplyFilters() and tryApplyProjections(). For *SupportsProjectionPushDown*, we still need a *supportsNestedProjections* API on the table source as some of the table sources might not be able to handle nested fields and therefore the Fl

Re: [DISCUSS] FLIP-356: Support Nested Fields Filter Pushdown

Hi Jark, Regarding the migration path, it would be useful to scrutinize the use case of FiledReferenceExpression and ResolvedExpressions. There are two kinds of use cases: 1. A ResolvedExpression is constructed by the user or connector / plugin developers. 2. A ResolvedExpression is constructed b

Re: [DISCUSS] FLIP-356: Support Nested Fields Filter Pushdown

Hi Becket, I totally agree we should try to have a consistent API for a final state. The only concern I have mentioned is the "smooth" migration path. The FiledReferenceExpression is widely used in many public APIs, not only in the SupportsFilterPushDown. Yes, we can change every methods in 2-step

Re: [DISCUSS] FLIP-356: Support Nested Fields Filter Pushdown

Thanks for the reply, Jark. I think it will be helpful to understand the final state we want to eventually achieve first, then we can discuss the steps towards that final state. It looks like there are two proposed end states now: 1. Have a separate NestedFieldReferenceExpression class; keep Sup

Re: [DISCUSS] FLIP-356: Support Nested Fields Filter Pushdown

Hi Venkat, Thanks for the proposal. I have some minor comments about the FLIP. 1. I think we don't need to add SupportsFilterPushDown#supportsNestedFilters() method, because connectors can skip nested filters by putting them in Result#remainingFilters(). And this is backward-compatible because u

Re: [DISCUSS] FLIP-356: Support Nested Fields Filter Pushdown

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 A

Re: [DISCUSS] FLIP-356: Support Nested Fields Filter Pushdown

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. Otherwise, the alternative solution briefly mentioned in the rejected alternatives would be the following: Phase 1: 1. Intr

Re: [DISCUSS] FLIP-356: Support Nested Fields Filter Pushdown

Gentle ping for reviews/feedback. On Tue, Aug 15, 2023, 5:37 PM Venkatakrishnan Sowrirajan wrote: > Hi All, > > I am opening this thread to discuss FLIP-356: Support Nested Fields > Filter Pushdown. The FLIP can be found at > https://cwiki.apache.org/confluence/display/FLINK/FLIP-356%3A+Support+