letian-jiang commented on code in PR #3086:
URL: https://github.com/apache/drill/pull/3086#discussion_r4390307593
##########
contrib/storage-hbase/src/main/java/org/apache/drill/exec/store/hbase/HBaseStoragePlugin.java:
##########
@@ -58,6 +65,26 @@ public boolean supportsRead() {
return true;
}
+ @Override
+ public boolean supportPlanCache(DrillTableSelection selection) {
+ return selection instanceof HBaseScanSpec;
+ }
+
+ @Override
+ public PlanCacheTable planCacheTable(DrillTableSelection selection) throws
IOException {
+ if (!supportPlanCache(selection)) {
+ return null;
+ }
+ String identifier = ((HBaseScanSpec) selection).getTableName();
+ try (Table table =
getConnection().getTable(TableName.valueOf(identifier))) {
+ String version = Hashing.sha256().hashBytes(
+
TableDescriptorBuilder.toByteArray(table.getDescriptor())).toString();
Review Comment:
Agreed. I’ll leave further metadata reuse and the existing Admin lifetime
issue for a follow-up optimization.
##########
contrib/storage-hbase/src/main/java/org/apache/drill/exec/store/hbase/HBaseGroupScan.java:
##########
@@ -108,18 +116,44 @@ public HBaseGroupScan(@JsonProperty("userName") String
userName,
@JsonProperty("storage") HBaseStoragePluginConfig
storagePluginConfig,
@JsonProperty("columns") List<SchemaPath> columns,
@JsonProperty("maxRecords") int maxRecords,
+ @JsonProperty("baseScanSpec") HBaseScanSpec
baseScanSpec,
+ @JsonProperty("pushedFilter") String pushedFilter,
+ @JsonProperty("pushedFilterFullyConverted") boolean
pushedFilterFullyConverted,
@JacksonInject StoragePluginRegistry pluginRegistry)
throws IOException, ExecutionSetupException {
- this (userName, pluginRegistry.resolve(storagePluginConfig,
HBaseStoragePlugin.class), hbaseScanSpec, columns, maxRecords);
+ this(userName, pluginRegistry.resolve(storagePluginConfig,
HBaseStoragePlugin.class),
+ hbaseScanSpec, columns, maxRecords, baseScanSpec,
+ pushedFilter == null ? null :
LogicalExpressionParser.parse(pushedFilter),
+ pushedFilterFullyConverted, true);
}
public HBaseGroupScan(String userName, HBaseStoragePlugin storagePlugin,
HBaseScanSpec scanSpec,
List<SchemaPath> columns, int maxRecords) {
+ this(userName, storagePlugin, scanSpec, columns, maxRecords, null, null,
false, false);
+ }
+
+ HBaseGroupScan(String userName, HBaseStoragePlugin storagePlugin,
HBaseScanSpec scanSpec,
+ List<SchemaPath> columns, int maxRecords, HBaseScanSpec baseScanSpec,
+ LogicalExpression pushedFilter, boolean pushedFilterFullyConverted,
boolean rebuild) {
super(userName);
this.storagePlugin = storagePlugin;
this.storagePluginConfig = storagePlugin.getConfig();
- this.hbaseScanSpec = scanSpec;
+ this.baseScanSpec = baseScanSpec;
+ this.pushedFilter = pushedFilter;
+ this.pushedFilterFullyConverted = pushedFilterFullyConverted;
+ this.hbaseScanSpec = rebuild && pushedFilter != null ? baseScanSpec :
scanSpec;
this.columns = columns == null ? ALL_COLUMNS : columns;
this.maxRecords = maxRecords;
+ if (rebuild && pushedFilter != null) {
+ if (baseScanSpec == null) {
+ throw new IllegalArgumentException("Cached HBase filter has no base
scan specification");
+ }
+ HBaseFilterBuilder builder = new HBaseFilterBuilder(this, pushedFilter);
+ HBaseScanSpec rebuilt = builder.parseTree();
+ if (rebuilt == null || (pushedFilterFullyConverted &&
!builder.isAllExpressionsConverted())) {
Review Comment:
Cache hits now require scan reconstruction; ordinary physical-plan JSON uses
its saved scan specification. Added tests for JSON replay, region splits,
partial filtering and parameter changes that require replanning.
##########
logical/src/main/java/org/apache/drill/common/expression/LiteralExpression.java:
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.drill.common.expression;
+
+import com.fasterxml.jackson.annotation.JsonIgnore;
+
+/** A literal that may carry a bound SQL parameter slot in a cached plan. */
+public abstract class LiteralExpression extends LogicalExpressionBase {
+ private int dynamicParamIndex = -1;
+
+ protected LiteralExpression(ExpressionPosition position) {
+ super(position);
+ }
+
+ @JsonIgnore
+ public int getDynamicParamIndex() {
+ return dynamicParamIndex;
+ }
+
+ /** Whether this literal is bound to a zero-based SQL parameter slot. */
+ @JsonIgnore
+ public boolean isDynamicParam() {
+ return dynamicParamIndex >= 0;
+ }
+
+ public void setDynamicParamIndex(int dynamicParamIndex) {
Review Comment:
Thanks for pointing this out. The current binding paths allocate fresh
literals and use VARDECIMAL for bound decimals. Given that these cases seem
unlikely in the current implementation, I’d prefer to keep the design simple
for now and revisit the additional hardening if those paths expand.
--
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]
