cgivre commented on code in PR #3000:
URL: https://github.com/apache/drill/pull/3000#discussion_r2200897541
##########
contrib/storage-hbase/src/main/java/org/apache/drill/exec/store/hbase/HBaseGroupScan.java:
##########
@@ -382,7 +402,7 @@ public String getDigest() {
public String toString() {
return "HBaseGroupScan [HBaseScanSpec="
Review Comment:
Can we update this method with the `PlanStringBuilder` as shown below?
Please update with appropriate fields
```
@Override
public String toString() {
return new PlanStringBuilder(this)
.field("HbaseScanSpec", hbaseScanSpec)
.field("columns", columns)
.field("maxRecords", maxRecords)
.toString();
}
```
##########
contrib/storage-hbase/src/main/java/org/apache/drill/exec/store/hbase/HBaseGroupScan.java:
##########
@@ -396,6 +416,33 @@ public List<SchemaPath> getColumns() {
return columns;
}
+ @JsonProperty
+ public int getMaxRecords() {
+ return maxRecords;
+ }
+
+ /**
+ * Default is not to support limit pushdown.
+ */
+ @Override
+ @JsonIgnore
Review Comment:
Is there a reason for the `JsonIgnore`?
##########
contrib/storage-hbase/src/main/java/org/apache/drill/exec/store/hbase/HBaseSubScan.java:
##########
@@ -98,6 +101,11 @@ public HBaseStoragePlugin getStorageEngine(){
return hbaseStoragePlugin;
}
+ @JsonIgnore
+ public int getMaxRecords() {
Review Comment:
Again is there a reason for the `JsonIgnore`?
##########
contrib/storage-hbase/src/main/java/org/apache/drill/exec/store/hbase/HBaseRecordReader.java:
##########
@@ -84,12 +84,16 @@ public class HBaseRecordReader extends AbstractRecordReader
implements DrillHBas
private final Connection connection;
- public HBaseRecordReader(Connection connection,
HBaseSubScan.HBaseSubScanSpec subScanSpec, List<SchemaPath> projectedColumns) {
+ public HBaseRecordReader(Connection connection,
HBaseSubScan.HBaseSubScanSpec subScanSpec, List<SchemaPath> projectedColumns,
int maxRecords) {
this.connection = connection;
hbaseTableName = TableName.valueOf(
Preconditions.checkNotNull(subScanSpec, "HBase reader needs a sub-scan
spec").getTableName());
hbaseScan = new Scan(subScanSpec.getStartRow(), subScanSpec.getStopRow());
hbaseScanColumnsOnly = new Scan();
+ //todo Whether to remove if condition judgment
Review Comment:
I'm not sure what this comment means but please remove or explain further.
Just so you are aware, Calcite does provide the logic as to when to push the
limit down and when not to so I think we are good here. To verify this, you
can add a unit test with the query:
```sql
SELECT *
FROM hbase
LIMIT 10 OFFSET 5
```
and you should see the `maxRecords` parameter set to 15.
If you look at the unit tests for the Phoenix plugin, you can see a good
example of this:
https://github.com/apache/drill/blob/78a52087bb85bcc7034157544193155b87503111/contrib/storage-phoenix/src/test/java/org/apache/drill/exec/store/phoenix/PhoenixSQLTest.java#L92-L128
--
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: dev-unsubscr...@drill.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
