etseidl commented on PR #11157: URL: https://github.com/apache/arrow-rs/pull/11157#issuecomment-5915492637
Thanks for the detailed review @alamb 🙏 I do want to get this right before merging, so I think it's worth discussing the replacement behavior. > I think some users may be bitten by this implicit change in behavior. So one issue is we unwittingly introduced a change in behavior in 60.0.0. Prior to that, the column and offset indexes were separate monolithic entities. The push decoder separately parsed them, so they were preserved on subsequent calls where the policy was `Skip`. If both were set to `Skip` there was an early return from `ParquetMetaDataPushDecoder::try_decode`. If only one policy was skip, then there was an early return from the index parser. So for a tuple of `(offset_policy, column_policy)` the behavior was ``` (skip, skip) -> no-op, both indexes preserved (skip, !skip) | (!skip, skip) -> preserve the skipped index, replace the other (!skip, !skip) -> replace both ``` 60.0.0 merged both indexes into a single `PageIndex`, which is now a new monolith. The current behavior then is ``` (skip, skip) -> no-op, both indexes preserved (skip, !skip) | (!skip, skip) -> clear the skipped index, replace the other (!skip, !skip) -> replace both ``` So we've already lost some of the preservation that existed prior. @adriangb's review pointed this out and how an early version of this PR exacerbated it. This is finding C11 above. > it sounds like you already tried to preserve the existing index and merge in the newly requested indexes and that got costly. I did, and the problem wasn't in this PR, but when changing the storage format. The current nested Vec storage allows for easy index merging since there is a slot already allocated for each cell. Subsequent calls could update the cells they target and leave the others alone. The problem really arises if we try to get fancy with storage and only allocate enough space for the requested subgrid; later trying to append columns or row groups requires a reallocation and move of the existing cells. But maybe that's putting the cart before the horse. The issue is with sparse indexes, why waste storage on things we don't want. The current nested vec is awful. We need to at least change to a single allocation and calculate positions manually. But an `Option<Index>` is pretty heavy. I did a quick `heap_size` on a 100 x 10000 empty index, and it was about 232MB. But if we `Box` or `Arc` the individual cells, this drops to only 8MB. Now that would add some overhead to a fully populated index, but it does allow for cheap upsert like behavior when incrementally building the indexes. Now that's still 8MB vs almost free if we only want a single column, for instance, but life's all about tradeoffs 😉. So perhaps the preserve path isn't so bad. As C11 points out, we need to do _something_ and document it. The previous behavior was not a contract, just a consequence of how things were implemented. 60.0.0 introduced a behavior change that we should either revert or document anyway. I guess at this point I'm still open to either path, either what I have now (always replace), or go back to preserve across multiple calls, and take that into account as we try to make the back-end storage more efficient. Sounds like @alamb is a vote for the latter; I abstain. Other votes? @adriangb, @zhuqi-lucas, @sunchao? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
