You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@kyuubi.apache.org by "echo567 (via GitHub)" <gi...@apache.org> on 2025/11/16 11:16:45 UTC
[PR] [KYUUBI apache#7245] fix arrow batch converter error [kyuubi]
echo567 opened a new pull request, #7246:
URL: https://github.com/apache/kyuubi/pull/7246
fix #7245
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #7245] Fix arrow batch converter error [kyuubi]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #7246:
URL: https://github.com/apache/kyuubi/pull/7246#issuecomment-3550553854
The code is copied from Spark, seems it was changed at [SPARK-44657](https://issues.apache.org/jira/browse/SPARK-44657). Can we just follow that?
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #7245] Fix arrow batch converter error [kyuubi]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #7246:
URL: https://github.com/apache/kyuubi/pull/7246#issuecomment-3542230324
## [Codecov](https://app.codecov.io/gh/apache/kyuubi/pull/7246?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `0%` with `6 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 0.00%. Comparing base ([`3b205a3`](https://app.codecov.io/gh/apache/kyuubi/commit/3b205a3924e0e3a75c425de1396089729cf22ee5?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`479d7e4`](https://app.codecov.io/gh/apache/kyuubi/commit/479d7e40bb99147045add38865e5872e21bb840b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 6 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/kyuubi/pull/7246?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...rk/sql/execution/arrow/KyuubiArrowConverters.scala](https://app.codecov.io/gh/apache/kyuubi/pull/7246?src=pr&el=tree&filepath=externals%2Fkyuubi-spark-sql-engine%2Fsrc%2Fmain%2Fscala%2Forg%2Fapache%2Fspark%2Fsql%2Fexecution%2Farrow%2FKyuubiArrowConverters.scala&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvc3Bhcmsvc3FsL2V4ZWN1dGlvbi9hcnJvdy9LeXV1YmlBcnJvd0NvbnZlcnRlcnMuc2NhbGE=) | 0.00% | [5 Missing :warning: ](https://app.codecov.io/gh/apache/kyuubi/pull/7246?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...g/apache/spark/sql/kyuubi/SparkDatasetHelper.scala](https://app.codecov.io/gh/apache/kyuubi/pull/7246?src=pr&el=tree&filepath=externals%2Fkyuubi-spark-sql-engine%2Fsrc%2Fmain%2Fscala%2Forg%2Fapache%2Fspark%2Fsql%2Fkyuubi%2FSparkDatasetHelper.scala&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvc3Bhcmsvc3FsL2t5dXViaS9TcGFya0RhdGFzZXRIZWxwZXIuc2NhbGE=) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/kyuubi/pull/7246?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #7246 +/- ##
======================================
Coverage 0.00% 0.00%
======================================
Files 696 696
Lines 43530 43527 -3
Branches 5883 5879 -4
======================================
+ Misses 43530 43527 -3
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/kyuubi/pull/7246?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #7245] Fix arrow batch converter error [kyuubi]
Posted by "echo567 (via GitHub)" <gi...@apache.org>.
echo567 commented on PR #7246:
URL: https://github.com/apache/kyuubi/pull/7246#issuecomment-3581057115
> The code is copied from Spark, seems it was changed at [SPARK-44657](https://issues.apache.org/jira/browse/SPARK-44657). Can we just follow that?
okay,I made modifications based on this Spark issue
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #7245] Fix arrow batch converter error [kyuubi]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #7246:
URL: https://github.com/apache/kyuubi/pull/7246#issuecomment-3690791154
thanks, merged to master/1.11/1.10
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #7245] Fix arrow batch converter error [kyuubi]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #7246:
URL: https://github.com/apache/kyuubi/pull/7246#discussion_r2645790522
##########
externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/spark/sql/execution/arrow/KyuubiArrowConverters.scala:
##########
@@ -274,19 +274,28 @@ object KyuubiArrowConverters extends SQLConfHelper with Logging {
var estimatedBatchSize = 0L
Utils.tryWithSafeFinally {
+ def isBatchSizeLimitExceeded: Boolean = {
+ // If `maxEstimatedBatchSize` is zero or negative, it implies unlimited.
+ maxEstimatedBatchSize > 0 && estimatedBatchSize >= maxEstimatedBatchSize
+ }
+ def isRecordLimitExceeded: Boolean = {
+ // If `maxRecordsPerBatch` is zero or negative, it implies unlimited.
+ maxRecordsPerBatch > 0 && rowCountInLastBatch >= maxRecordsPerBatch
+ }
+ def isGlobalLimitNotReached: Boolean = {
+ // If the limit is negative, it means no restriction
+ // or the current number of rows has not reached the limit.
+ rowCount < limit || limit < 0
+ }
+
// Always write the first row.
- while (rowIter.hasNext && (
- // For maxBatchSize and maxRecordsPerBatch, respect whatever smaller.
+ while (rowIter.hasNext && isGlobalLimitNotReached && (
// If the size in bytes is positive (set properly), always write the first row.
- rowCountInLastBatch == 0 && maxEstimatedBatchSize > 0 ||
- // If the size in bytes of rows are 0 or negative, unlimit it.
- estimatedBatchSize <= 0 ||
- estimatedBatchSize < maxEstimatedBatchSize ||
- // If the size of rows are 0 or negative, unlimit it.
- maxRecordsPerBatch <= 0 ||
- rowCountInLastBatch < maxRecordsPerBatch ||
- rowCount < limit ||
- limit < 0)) {
+ rowCountInLastBatch == 0 ||
+ // If either limit is hit, create a batch. This implies that the limit that is hit
+ // first triggers the creation of a batch even if the other limit is not yet hit
+ // hence preferring the more restrictive limit.
Review Comment:
The comment states "If either limit is hit, create a batch" but the logic actually continues the loop when neither limit is exceeded. The comment should be clarified to say "Continue adding rows to the batch until either limit is exceeded" or similar wording to accurately reflect the loop continuation condition rather than batch creation trigger.
```suggestion
// Continue adding rows to the current batch until either limit is exceeded.
// The limit that is reached first determines the batch boundary, even if the
// other limit has not yet been reached, thus preferring the more restrictive limit.
```
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #7245] Fix arrow batch converter error [kyuubi]
Posted by "echo567 (via GitHub)" <gi...@apache.org>.
echo567 commented on PR #7246:
URL: https://github.com/apache/kyuubi/pull/7246#issuecomment-3550420517
> @echo567, please keep the PR template and fill in seriously, especially "Was this patch authored or co-authored using generative AI tooling?", it does matter for legal purposes.
Sorry, the changes have been made.
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #7245] Fix arrow batch converter error [kyuubi]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #7246:
URL: https://github.com/apache/kyuubi/pull/7246#issuecomment-3541461310
@echo567, please keep the PR template and fill in seriously, especially "Was this patch authored or co-authored using generative AI tooling?", it does matter for legal purposes.
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #7245] Fix arrow batch converter error [kyuubi]
Posted by "echo567 (via GitHub)" <gi...@apache.org>.
echo567 commented on PR #7246:
URL: https://github.com/apache/kyuubi/pull/7246#issuecomment-3626881324
hi I've merged the code from the latest master branch. Is there anything else I need to change?
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #7245] Fix arrow batch converter error [kyuubi]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 closed pull request #7246: [KYUUBI #7245] Fix arrow batch converter error
URL: https://github.com/apache/kyuubi/pull/7246
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
Re: [PR] [KYUUBI #7245] Fix arrow batch converter error [kyuubi]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #7246:
URL: https://github.com/apache/kyuubi/pull/7246#issuecomment-3688666927
@cfmcgrady, do you want to have another look?
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org