gripleaf commented on PR #301:
URL: https://github.com/apache/paimon-cpp/pull/301#issuecomment-5612410435
> Thanks, the selection logic itself is correct and the test matrix is
thorough. Both earlier review comments are addressed at this head: the
TIMESTAMP/DECIMAL128 eager arms are gone and the benchmark moved to
`benchmark/`. My concerns are about the performance model rather than
correctness, and most of them come from the interaction with #306.
>
> ## The three commits have very different value
> `b9061c0` (`num_fields()`) is independent, tiny and obviously correct.
Please split it out with `TestLeafFieldBoundsOnWideSlicedBatch` and the
integration test into its own PR so it can merge on its own; there is no reason
to couple it to the selection work.
>
> `4785b98` (typed equality) is a strict subset of #306 and should be
dropped, see below.
>
> The selection commits are correct but their payoff shrinks a lot once #306
lands, and the benchmark as posted does not isolate the cost that decides
whether they are net positive.
Thanks for the review. I’ve split the num_fields() optimization and its unit
and integration tests into a separate branch as requested. I’ll close this PR
for now and evaluate the selection optimization against the latest main,
including #306.
I’ll open a new PR once I have benchmark results that isolate the
selection overhead and demonstrate a net performance benefit.
--
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]
