github-actions[bot] commented on code in PR #60982:
URL: https://github.com/apache/doris/pull/60982#discussion_r2909635156
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/BuiltinScalarFunctions.java:
##########
@@ -1087,8 +1098,9 @@ public class BuiltinScalarFunctions implements
FunctionHelper {
scalar(TimeFormat.class, "time_format"),
scalar(TimeToSec.class, "time_to_sec"),
scalar(Timestamp.class, "timestamp"),
- scalar(ToBase64.class, "to_base64"),
+ scalar(ToBase64.class, "to_base64", "base64_encode"),
scalar(ToBase64Binary.class, "to_base64_binary"),
+ scalar(ToVarcharFn.class, "to_varchar"),
scalar(ToBinary.class, "to_binary", "to_hex"),
Review Comment:
**Nit (Registration ordering):** `to_varchar` sorts after `to_seconds`
alphabetically. This registration should be placed after
`scalar(ToSeconds.class, ...)` rather than here between `to_base64_binary` and
`to_binary`.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/scalar/ObjectConstruct.java:
##########
@@ -0,0 +1,103 @@
+// 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.doris.nereids.trees.expressions.functions.scalar;
+
+import org.apache.doris.catalog.FunctionSignature;
+import org.apache.doris.nereids.exceptions.AnalysisException;
+import org.apache.doris.nereids.trees.expressions.Expression;
+import org.apache.doris.nereids.trees.expressions.functions.AlwaysNotNullable;
+import org.apache.doris.nereids.trees.expressions.functions.CustomSignature;
+import org.apache.doris.nereids.trees.expressions.functions.RewriteWhenAnalyze;
+import org.apache.doris.nereids.trees.expressions.visitor.ExpressionVisitor;
+import org.apache.doris.nereids.types.DataType;
+import org.apache.doris.nereids.types.JsonType;
+import org.apache.doris.nereids.types.VarcharType;
+import org.apache.doris.nereids.util.ExpressionUtils;
+
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * Snowflake-compatible function 'object_construct'.
+ * OBJECT_CONSTRUCT(key1, value1, key2, value2, ...) builds a JSON object.
+ *
+ * V1 approximation: Rewrites to json_object(key1, value1, key2, value2, ...).
+ * Note: Snowflake skips keys whose value is NULL; this V1 implementation
+ * retains "key": null in the output. V2 will implement precise NULL-skipping
+ * semantics via a new BE function.
+ */
+public class ObjectConstruct extends ScalarFunction
+ implements CustomSignature, AlwaysNotNullable, RewriteWhenAnalyze {
+
+ /**
+ * constructor with 0 or more arguments.
+ */
+ public ObjectConstruct(Expression... varArgs) {
+ super("object_construct", ExpressionUtils.mergeArguments(varArgs));
+ }
+
+ /** constructor for withChildren and reuse signature */
+ private ObjectConstruct(ScalarFunctionParams functionParams) {
+ super(functionParams);
+ }
+
+ @Override
+ public FunctionSignature customSignature() {
+ List<DataType> arguments = new ArrayList<>();
+ for (int i = 0; i < arity(); i++) {
+ if (i % 2 == 0) {
+ arguments.add(VarcharType.SYSTEM_DEFAULT);
+ } else {
+ arguments.add(getArgumentType(i));
+ }
+ }
+ return FunctionSignature.of(JsonType.INSTANCE, arguments);
+ }
+
+ @Override
+ public void checkLegalityBeforeTypeCoercion() {
+ if ((arity() & 1) == 1) {
+ throw new AnalysisException(
+ "object_construct requires even number of parameters
(key-value pairs): " + this.toSql());
+ }
+ for (int i = 0; i < arity(); i++) {
+ if ((i & 1) == 0 && getArgumentType(i).isNullType()) {
+ throw new AnalysisException("object_construct key can't be
NULL: " + this.toSql());
+ }
+ }
+ }
+
+ /**
+ * withChildren.
+ */
+ @Override
+ public ObjectConstruct withChildren(List<Expression> children) {
+ return new ObjectConstruct(getFunctionParams(children));
+ }
+
+ @Override
+ public <R, C> R accept(ExpressionVisitor<R, C> visitor, C context) {
+ return visitor.visitObjectConstruct(this, context);
+ }
+
+ @Override
+ public Expression rewriteWhenAnalyze() {
+ // V1: Delegate to json_object (does NOT skip NULL-valued keys)
+ return new JsonObject(children().toArray(new Expression[0]));
Review Comment:
**Bug (Critical):** `JsonObject` also implements `RewriteWhenAnalyze`. Its
`rewriteWhenAnalyze()` wraps non-JSON value arguments in `ToJson()`, which is
required because the BE `json_object` function (`FunctionJsonbObject`) rejects
non-JSONB value types with `InvalidArgument`. However, `ExpressionAnalyzer`
only checks `instanceof RewriteWhenAnalyze` once (lines 610-613) — the
`JsonObject` returned here will **not** have its `rewriteWhenAnalyze()` called.
This means `SELECT object_construct('key1', 123)` will fail at runtime
because the integer `123` won't be wrapped in `ToJson()`.
**Suggested fix:** Inline the `ToJson` wrapping logic here:
```java
@Override
public Expression rewriteWhenAnalyze() {
List<Expression> wrappedChildren = new ArrayList<>();
for (int i = 0; i < children().size(); i++) {
Expression child = child(i);
if (i % 2 == 0) {
wrappedChildren.add(child);
} else if (child.getDataType() instanceof JsonType) {
wrappedChildren.add(child);
} else {
wrappedChildren.add(new ToJson(child));
}
}
return new JsonObject(wrappedChildren.toArray(new Expression[0]));
}
```
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/BuiltinScalarFunctions.java:
##########
@@ -526,6 +531,7 @@
import
org.apache.doris.nereids.trees.expressions.functions.scalar.ToIpv6OrNull;
import org.apache.doris.nereids.trees.expressions.functions.scalar.ToIso8601;
import org.apache.doris.nereids.trees.expressions.functions.scalar.ToJson;
+import org.apache.doris.nereids.trees.expressions.functions.scalar.ToVarcharFn;
import org.apache.doris.nereids.trees.expressions.functions.scalar.ToMonday;
Review Comment:
**Nit (Import ordering):** `ToVarcharFn` sorts after
`ToMonday`/`ToQuantileState`/`ToSeconds` alphabetically. This import should be
moved after the `ToSeconds` import to maintain alphabetical order.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
