Only request is to keep the interface as generic as possible across all implementation. Think about chaining tables

havent changed the interface much. Mainly changed the Record

i think this will help in making it more generic

There are two upsert methods now?

yes. this was the point 4 in this list:

1. The Table interface will work off of Record class which will just be values.
2. Remove keys from record so selection/aggr/distinct all can use it.
3. We will endup having 3 impl Table with K-V (like HashMap), table with Keys (like HashSet), and table with values (like List). The impl's constructor can identify what id's in the Record are keys if needed.
4. For performance reasons, we might likely end up having two versions for upsert to avoid key generation multiple times.
5. This is to ensure that the same interface works across all layers in the stack.

Trying to understand that better.

Why is key generated outside? Projection should just create the record and add it right?

as of today, we are using Table in the CombineGroupByOrderByOperator, where key is generated by GroupKeyGenerator

I see, eventually we can simplify that?

yes. we do not know yet how things will look when we push Table into the segment level operator

another upsert might not be needed then

one thing i realized - in the lookup map, we now have

Key(Object[] keyValues)

and

Record(Object[] allvalues)

. We are using memory for

keyValues

twice Previously, it was

Key(Object[] keyValues)

and

Record(Key, Object[] aggregationValues)

. The Key within the

Record

was just a reference to the same

Key

object. But now, we have overhead for keys..

That’s ok for now. If we get the abstraction right, we can optimize further overtime

Jackie and Sidd have reviewed this. Did you also want to look @User and @User: https://github.com/apache/incubator-pinot/pull/4798 ?

looking into it

Will look at it after GCN

Made the changes based on the discussion we had earlier around multiple concrete Table implementations coming from BaseTable

<!here> can someone review this?

@User Reviewing now.

Seems OK overall, left a couple of minor comments. @User Since you are introducing the ResultSetter (that handles Distinct), will need to coordinate which PR goes first.

If that PR is still under review (which I think it is) and there are no pending comments on the above PR, then I think we can get this in

@User what do you think?

@User ^^

oh sorry, i thought we discussed offline that yours can go first

Yes, thanks. Do you have any comments? Some committer still needs to approve and merge. Mayank is good with it but didn't approve it

i havent had a chance to look at it with all the oncall stuff

@User has left the channel

@User has left the channel