I just realized that our query plans never really use arrays, but generally lists.

consistency nit: any reason to use arrays in LookupMergeDropExec (which necessitates this test change)? (Except that it's admittedly weird to not use int[].)

Other PhysicalPlans are not aware of channels - channels are an operator concept. (I know, in other query planning frameworks, the physical plans would already be aware of the physical layout of columnar data.)

Normally, we'd use attributes to refer to specific columns of the physical plan. Can't we do the same here?

The problem is the attributes are different in subsequent requests I think (they would have different id due to deserialization). Finding them by name might lead to performance issues. Any ideas how to go around those? Do you still think it is a bad idea to go with channels for now?

Looks like we only use mergingChannels in the mergePages == true case, that is, for ENRICH. Same for mergingTypes.

I think separating the enrich and lookup join code paths should get rid of this altogether, because we won't need them?

This is confusing. The MergePositionsOperator and ProjectOperator are quite different. Shouldn't/can't they be represented using different physical plans?

Well, we decide which one to use dynamically depending on the page contents and the optimization value. So for one page we might use MergePositionsOperator, for the other ProjectOperator. We cannot make that decision at Physical Planning because we don't have the page contents yet. The decision has to be made during Execution planning.

I think MergePositionOperator is only required for ENRICH, when we merge multiple matches into multivalues, no?

Looking through the code, this depends on mergePages, which is set during instantiation of the AbstractLookupService. For lookup join, we instantiate it with mergePages == false. Which means that the LookupMergeDropExec could just be a ProjectExec every time, no?

What is "the Block" in this context? Can we extend this javadoc, maybe also add an example?

I will add better comments. It is the Page that is passed in doQuery() that is sent at runtime.

In LocalExecutionPlanner, there's a little more machinery: we're building a LocalExecutionPlan there, which has driver factories, which in turn have driver suppliers, which then contain physical operations.

That doesn't apply to us because that machinery is aimed at building drivers based on shards, correct?

The query list requires the offset, which makes it pretty much operator level. But ParameterizedQueryExec contains a query list, which means we need to think about channels to make a ParameterizedQueryExec.

Is there a way to postpone the instantiation of the query list to the point where we perform the mapping to operators? Or another way to abstract so that can forget about channels until we hit the actual LookupExecutionMapper?

Yes, but the idea was that we do as much work as possible during Logical and Physical planning so it is done once and shared across requests. Also during the instantiation we decide what filters to push to Lucene and what to apply after. You need the filters to apply after for your Local Logical/Physical Optimization Rules. I am open to ideas though

Yes, but the idea was that we do as much work as possible during Logical and Physical planning so it is done once and shared across requests.

We're on the same page here! Assuming in the future, we'll be at a point where we stream pages to the driver(s), there should be an operator that has a QueryList, which has a channel offset and is re-used for each incoming page. 100%!

What I'm criticizing: This means QueryList is something that should exist at operator level, not inside a physical plan node. The ParameterizedQueryExec should not have to know anything about channels - it's the mapper's job to determine which input channel to use.

If we start making physical plans aware of channels, I'm pretty sure we'll end up with some weirdnesses that will start to proliferate and require workarounds. For instance, I won't be able to rewrite the query plan upstream from ParameterizedQueryExec (injecting an EvalExec, for instance, or projecting away a column that we realize we no longer need) without having to also update ParameterizedQueryExec because the upstream channels may change. This means that suddenly, the lookup physical optimizer will need to be aware of channels, which are supposed to be still abstracted away at this point :(

It's actually worse than that, because the query list not only is channel-aware, but it also has an alias filter which requires the indices service. It has shard contexts. And a Warnings object!

All of that is something that an operator would have, but not a physical plan.

In practical terms, I think ParameterizedQueryExec shouldn't contain a QueryList, but instead all the things that the lookup mapper needs to create the query list from.

Huh, do we need to create the warnings object in multiple places? There's another one that we make in createLookupPhysicalPlan.

Maybe we want to limit it to one warnings object that we pass around - or, even better, can we avoid needing the warnings object in createLookupPhysicalPlan if we manage to separate physical stuff out further from the query plan? This'd be the first time that a physical plan internally carries around a warnings object. Creating warnings is normally something that happens during runtime.

I will look into refactoring. It works right now though, the CSV tests pass with all warnings correct even though it is created in multiple places.

The fact that we need a shard context to create the physical plan also looks like operator-level objects seep into the planning layer. I'd expect shard contexts to only be needed once we map to operators.

If you compare with e.g. EsQueryExec, the physical plan that corresponds to running a Lucene query, that one is completely oblivious of shards.

This will need refactoring. I will work on it.

Do the dictionary/range block optimizations apply to lookup joins? Or just to enrich?

As it stands, the actual physical operators depends on the page that we perform the lookup for. Being per-page, this check would normally be inside an operator, not in the mapping stage.

Update: I see that mergePages is the differentiator between enrich and lookup join - being constantly false for the latter. So, for lookup joins, this step doesn't matter and we don't actually have to look into each page.

Since enrich doesn't need a proper planning stage, only lookup joins: how about we separate the code for the both more? Enforcing so much code share will be increasingly hard as we're re-architecting lookup join's planning and execution flow. If needed, they honestly don't even have to inherit from the same super class. That was useful a while ago, because lookup join re-used a ton of enrich code. But that's bound to change.

Right now dictionary/range optimization just apply to enrich. But I would like to take advantage of them for Lookup Join too in the future. So I did not want to rip this code out completely, and then have to put it back in when we want to apply this for optimization for Lookup Join too.

When we apply the dictionary/range optimizations, I'm pretty sure the code won't be transferable, anyway, because for ENRICH, the matching values get stuffed into multivalues, while for lookup join, multiple matching values become multiple new rows.

Let's separate the code paths. I think this refactor is currently quite complicated because it also refactors ENRICH code that's supposed to stay the way it is.

I think in the mergePages == false case, a lot of things simplify.