[GitHub] [iotdb] SteveYurongSu commented on a change in pull request #4257: [IOTDB-1886] Feature/const expr

[GitBox <gi...@apache.org>]

SteveYurongSu commented on a change in pull request #4257:
URL: https://github.com/apache/iotdb/pull/4257#discussion_r740692586



##########
File path: server/src/test/java/org/apache/iotdb/db/integration/IoTDBAlignByDeviceIT.java
##########
@@ -788,79 +788,6 @@ public void unusualCaseTest2() throws ClassNotFoundException {
     }
   }
 
-  @Test
-  public void selectConstantTest() throws ClassNotFoundException {
-    String[] retArray =
-        new String[] {
-          "1,root.vehicle.d0,101,1101,null,null,null,'11',",

Review comment:
       Please confirm if we need to modify the user doc :D
   
   

##########
File path: server/src/main/java/org/apache/iotdb/db/query/udf/core/transformer/ArithmeticBinaryTransformer.java
##########
@@ -36,6 +36,11 @@ protected ArithmeticBinaryTransformer(
     this.rightPointReader = rightPointReader;
   }
 
+  @Override
+  public boolean isConstantPointReader() {
+    return leftPointReader.isConstantPointReader() && rightPointReader.isConstantPointReader();

Review comment:
       It's better to cache the result of isConstantPointReader as a class member of Expression. Otherwise, a cachetime() call will cause multiple chain isConstantPointReader() calls.

##########
File path: server/src/main/java/org/apache/iotdb/db/query/udf/core/reader/ConstantLayerPointReader.java
##########
@@ -0,0 +1,117 @@
+/*
+ * 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.iotdb.db.query.udf.core.reader;
+
+import org.apache.iotdb.db.exception.query.QueryProcessException;
+import org.apache.iotdb.db.query.expression.unary.ConstantExpression;
+import org.apache.iotdb.tsfile.file.metadata.enums.TSDataType;
+import org.apache.iotdb.tsfile.utils.Binary;
+
+import org.apache.commons.lang3.Validate;
+
+import java.io.IOException;
+
+public class ConstantLayerPointReader implements LayerPointReader {
+
+  private final ConstantExpression expression;
+
+  public ConstantLayerPointReader(ConstantExpression expression) {
+    this.expression = Validate.notNull(expression);
+  }
+
+  @Override
+  public boolean isConstantPointReader() {
+    return true;
+  }
+
+  @Override
+  public boolean next() throws QueryProcessException, IOException {
+    return true;
+  }
+
+  @Override
+  public void readyForNext() {
+    // Do nothing
+  }
+
+  @Override
+  public TSDataType getDataType() {
+    return expression.getDataType();
+  }
+
+  @Override
+  public long currentTime() throws IOException {
+    throw new UnsupportedOperationException();
+  }
+
+  @Override
+  public int currentInt() throws IOException {
+    if (TSDataType.INT32.equals(expression.getDataType())) {
+      return (int) expression.getValue();

Review comment:
       It's better to avoid the unboxing process, which will cause heavy performance issues. You can refer to org/apache/iotdb/db/query/udf/core/transformer/Transformer.java to find the solution. :D

##########
File path: antlr/src/main/antlr4/org/apache/iotdb/db/qp/sql/IoTDBSqlLexer.g4
##########
@@ -915,6 +915,19 @@ fragment NAME_CHAR
     | CN_CHAR
     ;
 
+fragment FIRST_NAME_CHAR
+    : 'A'..'Z'
+    | 'a'..'z'
+    | '_'
+    | ':'
+    | '@'
+    | '#'
+    | '$'
+    | '{'
+    | '}'
+    |   CN_CHAR

Review comment:
       ```suggestion
       | CN_CHAR
   ```
   ```suggestion
       |   CN_CHAR
   ```

##########
File path: server/src/main/java/org/apache/iotdb/db/query/expression/unary/ConstantExpression.java
##########
@@ -0,0 +1,111 @@
+/*
+ * 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.iotdb.db.query.expression.unary;
+
+import org.apache.iotdb.db.exception.query.LogicalOptimizeException;
+import org.apache.iotdb.db.exception.query.QueryProcessException;
+import org.apache.iotdb.db.metadata.PartialPath;
+import org.apache.iotdb.db.qp.physical.crud.UDTFPlan;
+import org.apache.iotdb.db.qp.utils.WildcardsRemover;
+import org.apache.iotdb.db.query.expression.Expression;
+import org.apache.iotdb.db.query.udf.core.executor.UDTFExecutor;
+import org.apache.iotdb.db.query.udf.core.layer.ConstantIntermediateLayer;
+import org.apache.iotdb.db.query.udf.core.layer.IntermediateLayer;
+import org.apache.iotdb.db.query.udf.core.layer.LayerMemoryAssigner;
+import org.apache.iotdb.db.query.udf.core.layer.RawQueryInputLayer;
+import org.apache.iotdb.db.utils.CommonUtils;
+import org.apache.iotdb.tsfile.file.metadata.enums.TSDataType;
+
+import java.time.ZoneId;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+/** Constant expression */
+public class ConstantExpression extends Expression {
+
+  private final Object value;

Review comment:
       It's better to avoid the unboxing process, which will cause heavy performance issues. You can refer to org/apache/iotdb/db/query/udf/core/transformer/Transformer.java to find the solution. :D
   
   

##########
File path: server/src/main/java/org/apache/iotdb/db/query/dataset/UDTFAlignByTimeDataSet.java
##########
@@ -80,6 +80,9 @@ public UDTFAlignByTimeDataSet(
   protected void initTimeHeap() throws IOException, QueryProcessException {
     timeHeap = new TimeSelector(transformers.length << 1, true);
     for (LayerPointReader reader : transformers) {
+      if (reader.isConstantPointReader()) {
+        continue;
+      }

Review comment:
       It seems that the modification here is unnecessary because we have an assertion in parseResultColumn:
   
   ```java
   Expression expression = parseExpression(resultColumnContext.expression());
   if (expression.isPureConstantExpression()) {
     throw new SQLParserException("Pure constant expression is not allowed: " + expression);
   }
   ```

##########
File path: server/src/main/java/org/apache/iotdb/db/query/expression/unary/ConstantExpression.java
##########
@@ -0,0 +1,111 @@
+/*
+ * 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.iotdb.db.query.expression.unary;
+
+import org.apache.iotdb.db.exception.query.LogicalOptimizeException;
+import org.apache.iotdb.db.exception.query.QueryProcessException;
+import org.apache.iotdb.db.metadata.PartialPath;
+import org.apache.iotdb.db.qp.physical.crud.UDTFPlan;
+import org.apache.iotdb.db.qp.utils.WildcardsRemover;
+import org.apache.iotdb.db.query.expression.Expression;
+import org.apache.iotdb.db.query.udf.core.executor.UDTFExecutor;
+import org.apache.iotdb.db.query.udf.core.layer.ConstantIntermediateLayer;
+import org.apache.iotdb.db.query.udf.core.layer.IntermediateLayer;
+import org.apache.iotdb.db.query.udf.core.layer.LayerMemoryAssigner;
+import org.apache.iotdb.db.query.udf.core.layer.RawQueryInputLayer;
+import org.apache.iotdb.db.utils.CommonUtils;
+import org.apache.iotdb.tsfile.file.metadata.enums.TSDataType;
+
+import java.time.ZoneId;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+/** Constant expression */
+public class ConstantExpression extends Expression {
+
+  private final Object value;
+  private final TSDataType dataType;
+
+  public ConstantExpression(TSDataType dataType, String str) throws QueryProcessException {
+    this.dataType = dataType;
+    this.value = CommonUtils.parseValue(dataType, str);
+  }
+
+  public Object getValue() {
+    return value;
+  }
+
+  public TSDataType getDataType() {
+    return dataType;
+  }
+
+  @Override
+  public boolean isPureConstantExpression() {
+    return true;
+  }
+
+  @Override
+  public void concat(List<PartialPath> prefixPaths, List<Expression> resultExpressions) {
+    resultExpressions.add(this);

Review comment:
       This will not handle the following case:
   
   select sin(1 + s1) from root.sg.d1, root.sg.d2
   
   

##########
File path: server/src/main/java/org/apache/iotdb/db/query/expression/unary/ConstantExpression.java
##########
@@ -0,0 +1,111 @@
+/*
+ * 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.iotdb.db.query.expression.unary;
+
+import org.apache.iotdb.db.exception.query.LogicalOptimizeException;
+import org.apache.iotdb.db.exception.query.QueryProcessException;
+import org.apache.iotdb.db.metadata.PartialPath;
+import org.apache.iotdb.db.qp.physical.crud.UDTFPlan;
+import org.apache.iotdb.db.qp.utils.WildcardsRemover;
+import org.apache.iotdb.db.query.expression.Expression;
+import org.apache.iotdb.db.query.udf.core.executor.UDTFExecutor;
+import org.apache.iotdb.db.query.udf.core.layer.ConstantIntermediateLayer;
+import org.apache.iotdb.db.query.udf.core.layer.IntermediateLayer;
+import org.apache.iotdb.db.query.udf.core.layer.LayerMemoryAssigner;
+import org.apache.iotdb.db.query.udf.core.layer.RawQueryInputLayer;
+import org.apache.iotdb.db.utils.CommonUtils;
+import org.apache.iotdb.tsfile.file.metadata.enums.TSDataType;
+
+import java.time.ZoneId;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+/** Constant expression */
+public class ConstantExpression extends Expression {

Review comment:
       I think ConstantOperand may be a better name, cause we already have TimeSeriesOperand.

##########
File path: server/src/main/java/org/apache/iotdb/db/query/expression/unary/TimeSeriesOperand.java
##########
@@ -104,7 +109,7 @@ public IntermediateLayer constructIntermediateLayer(
 
       expressionIntermediateLayerMap.put(
           this,
-          memoryAssigner.getReference(this) == 1
+          memoryAssigner.getReference(this) == 1 || isPureConstantExpression()

Review comment:
       Why we construct a SingleInputColumnSingleReferenceIntermediateLayer when isPureConstantExpression() is true?

##########
File path: server/src/main/java/org/apache/iotdb/db/query/expression/unary/ConstantExpression.java
##########
@@ -0,0 +1,111 @@
+/*
+ * 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.iotdb.db.query.expression.unary;
+
+import org.apache.iotdb.db.exception.query.LogicalOptimizeException;
+import org.apache.iotdb.db.exception.query.QueryProcessException;
+import org.apache.iotdb.db.metadata.PartialPath;
+import org.apache.iotdb.db.qp.physical.crud.UDTFPlan;
+import org.apache.iotdb.db.qp.utils.WildcardsRemover;
+import org.apache.iotdb.db.query.expression.Expression;
+import org.apache.iotdb.db.query.udf.core.executor.UDTFExecutor;
+import org.apache.iotdb.db.query.udf.core.layer.ConstantIntermediateLayer;
+import org.apache.iotdb.db.query.udf.core.layer.IntermediateLayer;
+import org.apache.iotdb.db.query.udf.core.layer.LayerMemoryAssigner;
+import org.apache.iotdb.db.query.udf.core.layer.RawQueryInputLayer;
+import org.apache.iotdb.db.utils.CommonUtils;
+import org.apache.iotdb.tsfile.file.metadata.enums.TSDataType;
+
+import java.time.ZoneId;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+/** Constant expression */
+public class ConstantExpression extends Expression {
+
+  private final Object value;
+  private final TSDataType dataType;
+
+  public ConstantExpression(TSDataType dataType, String str) throws QueryProcessException {
+    this.dataType = dataType;
+    this.value = CommonUtils.parseValue(dataType, str);
+  }
+
+  public Object getValue() {
+    return value;
+  }
+
+  public TSDataType getDataType() {
+    return dataType;
+  }
+
+  @Override
+  public boolean isPureConstantExpression() {
+    return true;
+  }
+
+  @Override
+  public void concat(List<PartialPath> prefixPaths, List<Expression> resultExpressions) {
+    resultExpressions.add(this);
+  }
+
+  @Override
+  public void removeWildcards(WildcardsRemover wildcardsRemover, List<Expression> resultExpressions)
+      throws LogicalOptimizeException {
+    resultExpressions.add(this);
+  }
+
+  @Override
+  public void collectPaths(Set<PartialPath> pathSet) {
+    // Do nothing
+  }
+
+  @Override
+  public void constructUdfExecutors(
+      Map<String, UDTFExecutor> expressionName2Executor, ZoneId zoneId) {
+    // Do nothing
+  }
+
+  @Override
+  public void updateStatisticsForMemoryAssigner(LayerMemoryAssigner memoryAssigner) {
+    // Do nothing
+  }
+
+  @Override
+  public IntermediateLayer constructIntermediateLayer(
+      long queryId,
+      UDTFPlan udtfPlan,
+      RawQueryInputLayer rawTimeSeriesInputLayer,
+      Map<Expression, IntermediateLayer> expressionIntermediateLayerMap,
+      Map<Expression, TSDataType> expressionDataTypeMap,
+      LayerMemoryAssigner memoryAssigner) {
+    expressionDataTypeMap.put(this, this.getDataType());
+    IntermediateLayer intermediateLayer =
+        new ConstantIntermediateLayer(this, queryId, memoryAssigner.assign());
+    expressionIntermediateLayerMap.put(this, intermediateLayer);
+    return intermediateLayer;

Review comment:
       For 
   
   select sin(1 + s1), cos(1 + s1) from root.sg.d
   
   we'd better not construct the IntermediateLayer twice.




-- 
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: reviews-unsubscribe@iotdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org