Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dense reader: enable query condition on dimensions. #3887

Merged
merged 1 commit into from
Feb 16, 2023
Merged

Conversation

KiterLuc
Copy link
Contributor

This enables the dense reader to process query conditions on dimensions. By passing in the coordinates of each cell slabs down to the query condition layer, we can easily get the dimenion values for each cells at the query condition layer.


TYPE: IMPROVEMENT
DESC: Dense reader: enable query condition on dimensions.

This enables the dense reader to process query conditions on dimensions. By passing in the coordinates of each cell slabs down to the query condition layer, we can easily get the dimenion values for each cells at the query condition layer.

---
TYPE: IMPROVEMENT
DESC: Dense reader: enable query condition on dimensions.
@KiterLuc KiterLuc requested review from davisp and ihnorton February 12, 2023 09:57
Copy link
Contributor

@davisp davisp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most all of this makes sense. However, I have two main questions that I'm unsure about.

First and most shallow, I see we're iterating over dimensions differently based on the cell order which makes obvious sense. However, I can't decide if the iterating for COL_MAJOR is required or an optimization. I could see this being either an array of all the same value which means we don't need to bother referencing that data, or what is actually a single element array given that we know we don't have to hold an array when the dimension is constant. This isn't super important, but leads to my next question.

My second main question is whether we couldn't just add the data in cell_slab_coords into the ResultTile class somehow? I'd think that we'd probably have to add a bit more logic to that class rather than just plumbing code, but it seems like that'll be better long term compared adding yet more specialized logic to one of the three implementations. Granted, I'm suggesting that with a huge grain of salt that is the fact that I have no idea what sort of extension to ResultTile would be required. Though peeking at the API it appears we've already got coordinate related APIs in there so it doesn't strike me as super crazy to at least ask about it.

Copy link
Contributor

@davisp davisp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

I had a chat with @KiterLuc about this and he pointed out the ResultTile is shared by multiple threads applying query conditions at the same time, so setting dimension data on that is a no go without major refactoring, so I agree this is the best approach for now.

@KiterLuc KiterLuc merged commit 8a7242b into dev Feb 16, 2023
@KiterLuc KiterLuc deleted the lr/dense-qc-dim branch February 16, 2023 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants