tkalkirill commented on code in PR #13311:
URL: https://github.com/apache/ignite/pull/13311#discussion_r3712917696
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/LogicalRelImplementor.java:
##########
@@ -1050,4 +1054,33 @@ private ScanStorageNode<Row> createStorageScan(
otherColMapping
);
}
+
+ /** */
+ private long validateAndGetFetchOffsetParams(RexNode node, String op) {
Review Comment:
Let's look at an example: suppose you have two calls for `fetch`.
```
long offset = rel.offset() == null ? 0 :
validateAndGetFetchOffsetParams(rel.offset(), "offset");
long offset = rel.offset == null ? 0 :
validateAndGetFetchOffsetParams(rel.offset, "offset");
```
Issues:
1. Validation and value retrieval are combined in a single method, which
isn't exactly clean from an architectural standpoint.
2. The logic for handling a null is duplicated; the result is the same in
both cases, so this code duplication could be avoided.
3. The name implies that we are validating both FETCH and OFFSET
simultaneously, rather than just one or the other.
We can make an improvement, even if it's not perfect:
```
long offset = validateAndGetOffset(rel.offset());
long offset = validateAndGetOffset(rel.offset);
```
--
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]
