You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "xiedeyantu (via GitHub)" <gi...@apache.org> on 2025/12/26 13:44:08 UTC
[PR] [CALCITE-7346] Prevent overflow in metadata row-count when LIMIT/OFFSET literal exceeds Long range [calcite]
xiedeyantu opened a new pull request, #4707:
URL: https://github.com/apache/calcite/pull/4707
See [CALCITE-7346](https://issues.apache.org/jira/browse/CALCITE-7346)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7346] Prevent overflow in metadata row-count when LIMIT/OFFSET literal exceeds Long range [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4707:
URL: https://github.com/apache/calcite/pull/4707#discussion_r2648746593
##########
core/src/main/java/org/apache/calcite/rel/metadata/RelMdMaxRowCount.java:
##########
@@ -115,11 +117,10 @@ public Double getMaxRowCount(Sort rel, RelMetadataQuery mq) {
rowCount = Double.POSITIVE_INFINITY;
}
- final long offset = rel.offset instanceof RexLiteral ? RexLiteral.longValue(rel.offset) : 0;
+ final double offset = literalNumericValue(rel.offset, 0D);
Review Comment:
Pragmatically it is very unlikely that the code will deal with collections with more than 2^53 values.
Most metadata information is approximate anyway, counting on exact comparisons would be wrong.
But this should de considered independently for every kind of metadata.
(As a side note, in some computation modes which deal with multisets it is possible for multisets to have many copies of an element, exceeding 2^53 elements in a collection. In our implementation all sets are represented this way.)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7346] Prevent overflow in metadata row-count when LIMIT/OFFSET literal exceeds Long range [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4707:
URL: https://github.com/apache/calcite/pull/4707#discussion_r2649272648
##########
core/src/main/java/org/apache/calcite/rel/metadata/RelMdUtil.java:
##########
@@ -464,6 +464,32 @@ public static double capInfinity(Double d) {
return d.isInfinite() ? Double.MAX_VALUE : d;
}
+ /**
+ * Returns the numeric value stored in a literal as a double.
+ *
+ * <p>Throws when the literal exceeds {@link Double#MAX_VALUE} instead of
+ * silently rounding to infinity. Doubles still approximate large integers (53
+ * bits of mantissa), so the returned value becomes only an approximation when
+ * occupancy reaches that limit.
Review Comment:
when numbers are very large
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7346] Prevent overflow in metadata row-count when LIMIT/OFFSET literal exceeds Long range [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4707:
URL: https://github.com/apache/calcite/pull/4707#discussion_r2648755116
##########
core/src/main/java/org/apache/calcite/rel/metadata/RelMdUtil.java:
##########
@@ -464,6 +464,28 @@ public static double capInfinity(Double d) {
return d.isInfinite() ? Double.MAX_VALUE : d;
}
+ /**
+ * Returns the numeric value stored in a literal, throwing if it cannot be
+ * represented as a finite double.
+ */
+ public static double literalNumericValue(@Nullable RexNode node,
Review Comment:
this function should be called literalValueApproximatedByDouble
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7346] Prevent overflow in metadata row-count when LIMIT/OFFSET literal exceeds Long range [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu merged PR #4707:
URL: https://github.com/apache/calcite/pull/4707
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7346] Prevent overflow in metadata row-count when LIMIT/OFFSET literal exceeds Long range [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4707:
URL: https://github.com/apache/calcite/pull/4707#discussion_r2648737600
##########
core/src/main/java/org/apache/calcite/rel/metadata/RelMdMaxRowCount.java:
##########
@@ -115,11 +117,10 @@ public Double getMaxRowCount(Sort rel, RelMetadataQuery mq) {
rowCount = Double.POSITIVE_INFINITY;
}
- final long offset = rel.offset instanceof RexLiteral ? RexLiteral.longValue(rel.offset) : 0;
+ final double offset = literalNumericValue(rel.offset, 0D);
Review Comment:
Thank you for pointing that out. The currently used
"long" could indeed have the same issue. The current logic is all about metadata calculation (like row counts), and it’s actually quite easy for the number of rows to exceed the precision limit, especially in scenarios like the optimizer relying on row counts for cost estimation. If this logic doesn’t affect the actual computation of OFFSET/LIMIT in operators, I suppose it’s acceptable, but there is certainly a hidden risk.
Currently, the interfaces in these three classes all use
"Double" as the return type. Changing them to
"BigDecimal" would require significant modifications. Is it worth doing?
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7346] Prevent overflow in metadata row-count when LIMIT/OFFSET literal exceeds Long range [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4707:
URL: https://github.com/apache/calcite/pull/4707#discussion_r2648753506
##########
core/src/main/java/org/apache/calcite/rel/metadata/RelMdMaxRowCount.java:
##########
@@ -115,11 +117,10 @@ public Double getMaxRowCount(Sort rel, RelMetadataQuery mq) {
rowCount = Double.POSITIVE_INFINITY;
}
- final long offset = rel.offset instanceof RexLiteral ? RexLiteral.longValue(rel.offset) : 0;
+ final double offset = literalNumericValue(rel.offset, 0D);
Review Comment:
I may not have fully understood your point. Are you leaning towards accepting the changes in my current PR, or are you suggesting closing the PR? Currently, it only involves three metadata classes that compute RowCount.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7346] Prevent overflow in metadata row-count when LIMIT/OFFSET literal exceeds Long range [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4707:
URL: https://github.com/apache/calcite/pull/4707#discussion_r2649197051
##########
core/src/main/java/org/apache/calcite/rel/metadata/RelMdUtil.java:
##########
@@ -464,6 +464,28 @@ public static double capInfinity(Double d) {
return d.isInfinite() ? Double.MAX_VALUE : d;
}
+ /**
+ * Returns the numeric value stored in a literal, throwing if it cannot be
+ * represented as a finite double.
+ */
+ public static double literalNumericValue(@Nullable RexNode node,
Review Comment:
Please check if the modified code [496e6d8](https://github.com/apache/calcite/pull/4707/commits/496e6d8376463ae2ebea625596f5c39ab32ea6a4) meets the requirements of the comments.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7346] Prevent overflow in metadata row-count when LIMIT/OFFSET literal exceeds Long range [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4707:
URL: https://github.com/apache/calcite/pull/4707#discussion_r2648755450
##########
core/src/main/java/org/apache/calcite/rel/metadata/RelMdMaxRowCount.java:
##########
@@ -115,11 +117,10 @@ public Double getMaxRowCount(Sort rel, RelMetadataQuery mq) {
rowCount = Double.POSITIVE_INFINITY;
}
- final long offset = rel.offset instanceof RexLiteral ? RexLiteral.longValue(rel.offset) : 0;
+ final double offset = literalNumericValue(rel.offset, 0D);
Review Comment:
if long overflows, so does double, but the error handling is different: long will throw, but double will approximate.
Just make it clear what is happening.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7346] Prevent overflow in metadata row-count when LIMIT/OFFSET literal exceeds Long range [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4707:
URL: https://github.com/apache/calcite/pull/4707#discussion_r2648539722
##########
core/src/main/java/org/apache/calcite/rel/metadata/RelMdMaxRowCount.java:
##########
@@ -115,11 +117,10 @@ public Double getMaxRowCount(Sort rel, RelMetadataQuery mq) {
rowCount = Double.POSITIVE_INFINITY;
}
- final long offset = rel.offset instanceof RexLiteral ? RexLiteral.longValue(rel.offset) : 0;
+ final double offset = literalNumericValue(rel.offset, 0D);
Review Comment:
A large long value cannot be represented exactly as a double, since doubles have only 53 bits of mantissa.
In particular, very large integer values that differ may have the same representation as doubles.
Does this change the semantics of this code?
BigDecimal should be completely safe.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org