You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "hantangwangd (via GitHub)" <gi...@apache.org> on 2025/02/08 08:17:26 UTC
[PR] Core: Fix divide by zero when adjust split size [iceberg]
hantangwangd opened a new pull request, #12201:
URL: https://github.com/apache/iceberg/pull/12201
Currently, `TableScanUtil.adjustSplitSize(...)` can appropriately handle the situations where parameter `splitSize` is negative, however, it doesn't consider the situation where `splitSize` equals 0. That is to say, in a calculation engine like Spark, if we query a table after set it's property `read.split.target-size` to a negative value like `-1`, we will succeed and get the result, since the default `read.split.adaptive-size.enabled` is true. Meanwhile, if we query a table after set it's property `read.split.target-size` to 0, we will fail with the error message `/ by zero`.
This PR fix the inappropriate handling of the 0 value of `splitSize`, ensure that both negative and zero values can be adjusted to appropriate split size.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix divide by zero when adjust split size [iceberg]
Posted by "hantangwangd (via GitHub)" <gi...@apache.org>.
hantangwangd commented on code in PR #12201:
URL: https://github.com/apache/iceberg/pull/12201#discussion_r1955038335
##########
core/src/main/java/org/apache/iceberg/util/TableScanUtil.java:
##########
@@ -233,6 +233,8 @@ public static <T extends ScanTask> List<T> mergeTasks(List<T> tasks) {
}
public static long adjustSplitSize(long scanSize, int parallelism, long splitSize) {
+ Preconditions.checkArgument(splitSize > 0, "Split size must be > 0: %s", splitSize);
+
Review Comment:
Sure, I have added the check for `parallelism`, as well as a couple of unit tests for it.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix divide by zero when adjust split size [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #12201:
URL: https://github.com/apache/iceberg/pull/12201#discussion_r1947861228
##########
core/src/main/java/org/apache/iceberg/util/TableScanUtil.java:
##########
@@ -236,6 +236,9 @@ public static long adjustSplitSize(long scanSize, int parallelism, long splitSiz
// use the configured split size if it produces at least one split per slot
// otherwise, adjust the split size to target parallelism with a reasonable minimum
// increasing the split size may cause expensive spills and is not done automatically
+ if (splitSize <= 0) {
Review Comment:
Why are we allowing this at all? Shouldn't we just throw an error on Split size less than or = to 0?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix divide by zero when adjust split size [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #12201:
URL: https://github.com/apache/iceberg/pull/12201#discussion_r1954976117
##########
core/src/main/java/org/apache/iceberg/util/TableScanUtil.java:
##########
@@ -233,6 +233,8 @@ public static <T extends ScanTask> List<T> mergeTasks(List<T> tasks) {
}
public static long adjustSplitSize(long scanSize, int parallelism, long splitSize) {
+ Preconditions.checkArgument(splitSize > 0, "Split size must be > 0: %s", splitSize);
+
Review Comment:
This is a public api, so anyone can call it. We probably should still guard unless we are ok with it throwing divide by 0 exceptions
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix divide by zero when adjust split size [iceberg]
Posted by "hantangwangd (via GitHub)" <gi...@apache.org>.
hantangwangd commented on code in PR #12201:
URL: https://github.com/apache/iceberg/pull/12201#discussion_r1955038689
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/sql/TestSelect.java:
##########
@@ -57,7 +60,10 @@ protected static Object[][] parameters() {
{
SparkCatalogConfig.HIVE.catalogName(),
SparkCatalogConfig.HIVE.implementation(),
- SparkCatalogConfig.HIVE.properties(),
+ ImmutableMap.builder()
Review Comment:
Thanks for this information, it's a more suitable solution. The code has been modified, please take a look when available.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix divide by zero when adjust split size [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #12201:
URL: https://github.com/apache/iceberg/pull/12201#discussion_r1947961173
##########
core/src/main/java/org/apache/iceberg/util/TableScanUtil.java:
##########
@@ -236,6 +236,9 @@ public static long adjustSplitSize(long scanSize, int parallelism, long splitSiz
// use the configured split size if it produces at least one split per slot
// otherwise, adjust the split size to target parallelism with a reasonable minimum
// increasing the split size may cause expensive spills and is not done automatically
+ if (splitSize <= 0) {
Review Comment:
The general rule of thumb has been that we generally try not to throw errors on reading things out of metadata.json to avoid bricking on a metadata.json written by bad writer. This shouldn't be the case here since the metadata.json will parse correctly, but the invalid parameters would throw an error at runtime.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix divide by zero when adjust split size [iceberg]
Posted by "hantangwangd (via GitHub)" <gi...@apache.org>.
hantangwangd commented on code in PR #12201:
URL: https://github.com/apache/iceberg/pull/12201#discussion_r1947986708
##########
core/src/main/java/org/apache/iceberg/util/TableScanUtil.java:
##########
@@ -236,6 +236,9 @@ public static long adjustSplitSize(long scanSize, int parallelism, long splitSiz
// use the configured split size if it produces at least one split per slot
// otherwise, adjust the split size to target parallelism with a reasonable minimum
// increasing the split size may cause expensive spills and is not done automatically
+ if (splitSize <= 0) {
Review Comment:
When querying in Spark, the `adjustSplitSize(...)` is invoked earlier than `TableScanUtil.planTaskGroups(...)` as follows, so the illegal split size will be handle in `adjustSplitSize(...)` first and meet this problem. I believe in other engine, if they have the logic to adjust the split size, this would be the case as well.
```
TableScanUtil.planTaskGroups(
CloseableIterable.withNoopClose(tasks()),
adjustSplitSize(tasks(), scan.targetSplitSize()),
scan.splitLookback(),
scan.splitOpenFileCost());
```
According to my understanding of what you mead, we should simply add a `checkArgument` for splitSize in `adjustSplitSize(...)` to throw an error for illegal value, is that right?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix divide by zero when adjust split size [iceberg]
Posted by "hantangwangd (via GitHub)" <gi...@apache.org>.
hantangwangd commented on code in PR #12201:
URL: https://github.com/apache/iceberg/pull/12201#discussion_r1947986708
##########
core/src/main/java/org/apache/iceberg/util/TableScanUtil.java:
##########
@@ -236,6 +236,9 @@ public static long adjustSplitSize(long scanSize, int parallelism, long splitSiz
// use the configured split size if it produces at least one split per slot
// otherwise, adjust the split size to target parallelism with a reasonable minimum
// increasing the split size may cause expensive spills and is not done automatically
+ if (splitSize <= 0) {
Review Comment:
When querying in Spark, the `adjustSplitSize(...)` is invoked earlier than `TableScanUtil.planTaskGroups(...)` as follows, so the illegal split size will be handle in `adjustSplitSize(...)` first and meet this problem. I believe in other engine, if they have the logic to adjust the split size, this would be the case as well.
```
TableScanUtil.planTaskGroups(
CloseableIterable.withNoopClose(tasks()),
adjustSplitSize(tasks(), scan.targetSplitSize()),
scan.splitLookback(),
scan.splitOpenFileCost());
```
According to my understanding of what you mean, we should simply add a `checkArgument` for splitSize in `adjustSplitSize(...)` to throw an error for illegal value, is that right?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix divide by zero when adjust split size [iceberg]
Posted by "manuzhang (via GitHub)" <gi...@apache.org>.
manuzhang commented on code in PR #12201:
URL: https://github.com/apache/iceberg/pull/12201#discussion_r1947553534
##########
core/src/main/java/org/apache/iceberg/util/TableScanUtil.java:
##########
@@ -236,8 +236,12 @@ public static long adjustSplitSize(long scanSize, int parallelism, long splitSiz
// use the configured split size if it produces at least one split per slot
// otherwise, adjust the split size to target parallelism with a reasonable minimum
// increasing the split size may cause expensive spills and is not done automatically
+ long adjustedSplitSize = scanSize / parallelism;
+ if (splitSize <= 0) {
+ return adjustedSplitSize;
+ }
long splitCount = LongMath.divide(scanSize, splitSize, RoundingMode.CEILING);
- long adjustedSplitSize = Math.max(scanSize / parallelism, Math.min(MIN_SPLIT_SIZE, splitSize));
+ adjustedSplitSize = Math.max(adjustedSplitSize, Math.min(MIN_SPLIT_SIZE, splitSize));
Review Comment:
better to put all logic to compute `adjustedSplitSize` in a single function.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix divide by zero when adjust split size [iceberg]
Posted by "hantangwangd (via GitHub)" <gi...@apache.org>.
hantangwangd commented on PR #12201:
URL: https://github.com/apache/iceberg/pull/12201#issuecomment-2657960091
@RussellSpitzer my pleasure.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix divide by zero when adjust split size [iceberg]
Posted by "hantangwangd (via GitHub)" <gi...@apache.org>.
hantangwangd commented on code in PR #12201:
URL: https://github.com/apache/iceberg/pull/12201#discussion_r1947899223
##########
core/src/main/java/org/apache/iceberg/util/TableScanUtil.java:
##########
@@ -236,6 +236,9 @@ public static long adjustSplitSize(long scanSize, int parallelism, long splitSiz
// use the configured split size if it produces at least one split per slot
// otherwise, adjust the split size to target parallelism with a reasonable minimum
// increasing the split size may cause expensive spills and is not done automatically
+ if (splitSize <= 0) {
Review Comment:
FYI, I see for some other property, like `METADATA_PREVIOUS_VERSIONS_MAX`, will be adjust to a normal value when it's less than or equal to 0,
```
int maxSize =
Math.max(
1,
PropertyUtil.propertyAsInt(
properties,
TableProperties.METADATA_PREVIOUS_VERSIONS_MAX,
TableProperties.METADATA_PREVIOUS_VERSIONS_MAX_DEFAULT));
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix divide by zero when adjust split size [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #12201:
URL: https://github.com/apache/iceberg/pull/12201#discussion_r1949883612
##########
core/src/main/java/org/apache/iceberg/util/TableScanUtil.java:
##########
@@ -233,6 +233,8 @@ public static <T extends ScanTask> List<T> mergeTasks(List<T> tasks) {
}
public static long adjustSplitSize(long scanSize, int parallelism, long splitSize) {
+ Preconditions.checkArgument(splitSize > 0, "Split size must be > 0: %s", splitSize);
+
Review Comment:
Do we also need a check for parallelism? or is that checked elsewhere?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix divide by zero when adjust split size [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on PR #12201:
URL: https://github.com/apache/iceberg/pull/12201#issuecomment-2657564295
Thanks @hantangwangd for the PR!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix divide by zero when adjust split size [iceberg]
Posted by "hantangwangd (via GitHub)" <gi...@apache.org>.
hantangwangd commented on code in PR #12201:
URL: https://github.com/apache/iceberg/pull/12201#discussion_r1947894850
##########
core/src/main/java/org/apache/iceberg/util/TableScanUtil.java:
##########
@@ -236,6 +236,9 @@ public static long adjustSplitSize(long scanSize, int parallelism, long splitSiz
// use the configured split size if it produces at least one split per slot
// otherwise, adjust the split size to target parallelism with a reasonable minimum
// increasing the split size may cause expensive spills and is not done automatically
+ if (splitSize <= 0) {
Review Comment:
Thanks for the feedback @RussellSpitzer. I just saw that when trying to adjust the split size, negative values were adjusted to normal values, while the zero value threw a `/ by zero` exception due to being used as a divisor. So I tried to unify their behavior, whether it's uniformly adjusting to a normal split size or uniformly throwing an error.
If you think it's more appropriate to throw an error uniformly, I can adjust it. Please let me know your thoughts, thanks a lot.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix divide by zero when adjust split size [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #12201:
URL: https://github.com/apache/iceberg/pull/12201#discussion_r1950014100
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/sql/TestSelect.java:
##########
@@ -57,7 +60,10 @@ protected static Object[][] parameters() {
{
SparkCatalogConfig.HIVE.catalogName(),
SparkCatalogConfig.HIVE.implementation(),
- SparkCatalogConfig.HIVE.properties(),
+ ImmutableMap.builder()
Review Comment:
Why this 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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix divide by zero when adjust split size [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer merged PR #12201:
URL: https://github.com/apache/iceberg/pull/12201
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix divide by zero when adjust split size [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #12201:
URL: https://github.com/apache/iceberg/pull/12201#discussion_r1947961340
##########
core/src/main/java/org/apache/iceberg/util/TableScanUtil.java:
##########
@@ -236,6 +236,9 @@ public static long adjustSplitSize(long scanSize, int parallelism, long splitSiz
// use the configured split size if it produces at least one split per slot
// otherwise, adjust the split size to target parallelism with a reasonable minimum
// increasing the split size may cause expensive spills and is not done automatically
+ if (splitSize <= 0) {
Review Comment:
We already have this check immediately below,
```java
private static void validatePlanningArguments(long splitSize, int lookback, long openFileCost) {
Preconditions.checkArgument(splitSize > 0, "Split size must be > 0: %s", splitSize);
Preconditions.checkArgument(lookback > 0, "Split planning lookback must be > 0: %s", lookback);
Preconditions.checkArgument(openFileCost >= 0, "File open cost must be >= 0: %s", openFileCost);
}
```
How is this being avoided?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix divide by zero when adjust split size [iceberg]
Posted by "hantangwangd (via GitHub)" <gi...@apache.org>.
hantangwangd commented on code in PR #12201:
URL: https://github.com/apache/iceberg/pull/12201#discussion_r1950079317
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/sql/TestSelect.java:
##########
@@ -57,7 +60,10 @@ protected static Object[][] parameters() {
{
SparkCatalogConfig.HIVE.catalogName(),
SparkCatalogConfig.HIVE.implementation(),
- SparkCatalogConfig.HIVE.properties(),
+ ImmutableMap.builder()
Review Comment:
If we enable the cache in hive catalog (which is the default behavior), we cannot get the newest table metadata when executing a query closely after the action of setting table properties. In this case, in our test, the query `sql("SELECT * FROM %s", tableName)` will succeed after `table.updateProperties().set(SPLIT_SIZE, "-1").commit()`, that's not the expected behavior.
So I disabled the cache for hive catalog.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix divide by zero when adjust split size [iceberg]
Posted by "hantangwangd (via GitHub)" <gi...@apache.org>.
hantangwangd commented on code in PR #12201:
URL: https://github.com/apache/iceberg/pull/12201#discussion_r1947556326
##########
core/src/main/java/org/apache/iceberg/util/TableScanUtil.java:
##########
@@ -236,8 +236,12 @@ public static long adjustSplitSize(long scanSize, int parallelism, long splitSiz
// use the configured split size if it produces at least one split per slot
// otherwise, adjust the split size to target parallelism with a reasonable minimum
// increasing the split size may cause expensive spills and is not done automatically
+ long adjustedSplitSize = scanSize / parallelism;
+ if (splitSize <= 0) {
+ return adjustedSplitSize;
+ }
long splitCount = LongMath.divide(scanSize, splitSize, RoundingMode.CEILING);
- long adjustedSplitSize = Math.max(scanSize / parallelism, Math.min(MIN_SPLIT_SIZE, splitSize));
+ adjustedSplitSize = Math.max(adjustedSplitSize, Math.min(MIN_SPLIT_SIZE, splitSize));
Review Comment:
Thanks for your review. I have refactored to make it more clear, please take a look when available, thanks.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix divide by zero when adjust split size [iceberg]
Posted by "hantangwangd (via GitHub)" <gi...@apache.org>.
hantangwangd commented on code in PR #12201:
URL: https://github.com/apache/iceberg/pull/12201#discussion_r1950073181
##########
core/src/main/java/org/apache/iceberg/util/TableScanUtil.java:
##########
@@ -233,6 +233,8 @@ public static <T extends ScanTask> List<T> mergeTasks(List<T> tasks) {
}
public static long adjustSplitSize(long scanSize, int parallelism, long splitSize) {
+ Preconditions.checkArgument(splitSize > 0, "Split size must be > 0: %s", splitSize);
+
Review Comment:
It has been checked in Spark, if we set `parallelism <= 0` through `spark.conf().set("spark.sql.shuffle.partitions", "-1")`, we would fail with the following error message:
```
'0' in spark.sql.shuffle.partitions is invalid. The value of spark.sql.shuffle.partitions must be positive
'-1' in spark.sql.shuffle.partitions is invalid. The value of spark.sql.shuffle.partitions must be positive
```
So in Spark, `parallelism` would be a positive number here.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix divide by zero when adjust split size [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #12201:
URL: https://github.com/apache/iceberg/pull/12201#discussion_r1954977506
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/sql/TestSelect.java:
##########
@@ -57,7 +60,10 @@ protected static Object[][] parameters() {
{
SparkCatalogConfig.HIVE.catalogName(),
SparkCatalogConfig.HIVE.implementation(),
- SparkCatalogConfig.HIVE.properties(),
+ ImmutableMap.builder()
Review Comment:
This is not needed. You can invoke spark.sql("REFRESH TABLE ...)" which will let us avoid changing the config for all the other tests here.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Fix divide by zero when adjust split size [iceberg]
Posted by "hantangwangd (via GitHub)" <gi...@apache.org>.
hantangwangd commented on PR #12201:
URL: https://github.com/apache/iceberg/pull/12201#issuecomment-2648766365
Hi @RussellSpitzer, I have changed the code to check and uniformly throw an error when splitSize is less than or equal to 0, and adjust the relevant tests according to this behavior. Can you help review this? Thanks a lot.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org