Implement dense dimension aggregates. #4801
Conversation
This implements aggregates support with dense dimensions. --- TYPE: IMPROVEMENT DESC: Implement dense dimension aggregates.
|
This pull request has been linked to Shortcut Story #42340: Aggregates on dimension for dense arrays return 0. |
There was a problem hiding this comment.
Thanks for the great work @KiterLuc! Minor optimizations below. I'd like first to clarify why the current test for sparse dimensions didn't catch the bug you fixed, then ok to merge.
| const auto attribute = array_schema_.attribute(name); | ||
| const auto var_size = array_schema_.var_size(name); | ||
| const auto nullable = attribute->nullable(); | ||
| const auto nullable = attribute != nullptr && attribute->nullable(); |
There was a problem hiding this comment.
One of the callers up the stack makes sure name exists, let me know if I'm missing something, but the nullptr check seems redundant.
There was a problem hiding this comment.
Dimensions will not work properly here. But I changed the condition to clarify.
| md_names_to_load.emplace_back(name); | ||
| } | ||
| } | ||
| load_tile_metadata(relevant_fragments, md_names_to_load); |
There was a problem hiding this comment.
Any idea how this bug reproduced and why the test in #4794 didn't catch it?
There was a problem hiding this comment.
I think at some point your test stopped working because use_dim_ was never true?
There was a problem hiding this comment.
Hmm, I remember testing with and without the fix to see if crash happens, but I don't have any other explanation for now
This implements aggregates support with dense dimensions.
TYPE: IMPROVEMENT
DESC: Implement dense dimension aggregates.