Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
panbingkun commented on PR #45714: URL: https://github.com/apache/spark/pull/45714#issuecomment-2025209682 > thanks, merging to master! @cloud-fan Thanks for reviewing! -- 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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
cloud-fan closed pull request #45714: [SPARK-47559][SQL] Codegen Support for variant `parse_json` URL: https://github.com/apache/spark/pull/45714 -- 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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
cloud-fan commented on PR #45714: URL: https://github.com/apache/spark/pull/45714#issuecomment-2025079800 thanks, merging to master! -- 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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
panbingkun commented on PR #45714: URL: https://github.com/apache/spark/pull/45714#issuecomment-2024978712 > > it can be compatible with the invocation of methods with exception throwing signatures. > > `StaticInvoke` only calls internal methods and we can do this improvement if there is a use case. Okay. I think `VariantExpressionEvalUtils ` can be written in java after improvement, I'll try it later. Thanks! -- 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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
cloud-fan commented on PR #45714: URL: https://github.com/apache/spark/pull/45714#issuecomment-2024915514 > it can be compatible with the invocation of methods with exception throwing signatures. `StaticInvoke` only calls internal methods and we can do this improvement if there is a use case. -- 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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
panbingkun closed pull request #45744: [SPARK-47559][SQL] Codegen Support for variant `parse_json` URL: https://github.com/apache/spark/pull/45744 -- 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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
panbingkun commented on PR #45714: URL: https://github.com/apache/spark/pull/45714#issuecomment-2024562154 @cloud-fan After this pr, I also want to improve `StaticInvoke` in a separate PR so that it can be `compatible with` the invocation of methods with `exception throwing signatures`. Do you think it is necessary? -- 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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
panbingkun commented on PR #45744: URL: https://github.com/apache/spark/pull/45744#issuecomment-2024464908 I also want to improve `StaticInvoke` in `a separate PR` so that it can be `compatible with` the invocation of methods with exception throwing signatures. Do you think it is necessary? -- 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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
panbingkun commented on PR #45744: URL: https://github.com/apache/spark/pull/45744#issuecomment-2024460329 @cloud-fan I'm very sorry, I broke the branch corresponding to the previously reviewed PR. Can I use this new PR to submit this feature? The reviewed PR: https://github.com/apache/spark/pull/45714 -- 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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
panbingkun commented on code in PR #45714:
URL: https://github.com/apache/spark/pull/45714#discussion_r1542312888
##
sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/variant/VariantExpressionEvalUtils.java:
##
@@ -0,0 +1,52 @@
+/*
+ * 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.spark.sql.catalyst.expressions.variant;
+
+import scala.util.control.NonFatal;
+
+import org.apache.spark.sql.catalyst.InternalRow;
+import org.apache.spark.sql.catalyst.util.BadRecordException;
+import org.apache.spark.sql.errors.QueryExecutionErrors;
+import org.apache.spark.types.variant.Variant;
+import org.apache.spark.types.variant.VariantUtil;
+import org.apache.spark.types.variant.VariantBuilder;
+import org.apache.spark.types.variant.VariantSizeLimitException;
+import org.apache.spark.unsafe.types.UTF8String;
+import org.apache.spark.unsafe.types.VariantVal;
+
+/**
+ * A utility class for constructing variant expressions.
+ */
+public class VariantExpressionEvalUtils {
+
+ public static VariantVal parseJson(UTF8String input) {
+try {
+ Variant v = VariantBuilder.parseJson(input.toString());
+ return new VariantVal(v.getValue(), v.getMetadata());
+} catch (VariantSizeLimitException e) {
+ throw QueryExecutionErrors.variantSizeLimitError(VariantUtil.SIZE_LIMIT,
"parse_json");
+} catch (Throwable throwable) {
+ if (NonFatal.apply(throwable)) {
+throw
QueryExecutionErrors.malformedRecordsDetectedInRecordParsingError(
+input.toString(),
+new BadRecordException(() -> input, () -> new InternalRow[0],
throwable));
+ }
+ throw new Error(throwable);
Review Comment:
Okay
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
cloud-fan commented on code in PR #45714:
URL: https://github.com/apache/spark/pull/45714#discussion_r1542277307
##
sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/variant/VariantExpressionEvalUtils.java:
##
@@ -0,0 +1,52 @@
+/*
+ * 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.spark.sql.catalyst.expressions.variant;
+
+import scala.util.control.NonFatal;
+
+import org.apache.spark.sql.catalyst.InternalRow;
+import org.apache.spark.sql.catalyst.util.BadRecordException;
+import org.apache.spark.sql.errors.QueryExecutionErrors;
+import org.apache.spark.types.variant.Variant;
+import org.apache.spark.types.variant.VariantUtil;
+import org.apache.spark.types.variant.VariantBuilder;
+import org.apache.spark.types.variant.VariantSizeLimitException;
+import org.apache.spark.unsafe.types.UTF8String;
+import org.apache.spark.unsafe.types.VariantVal;
+
+/**
+ * A utility class for constructing variant expressions.
+ */
+public class VariantExpressionEvalUtils {
+
+ public static VariantVal parseJson(UTF8String input) {
+try {
+ Variant v = VariantBuilder.parseJson(input.toString());
+ return new VariantVal(v.getValue(), v.getMetadata());
+} catch (VariantSizeLimitException e) {
+ throw QueryExecutionErrors.variantSizeLimitError(VariantUtil.SIZE_LIMIT,
"parse_json");
+} catch (Throwable throwable) {
+ if (NonFatal.apply(throwable)) {
+throw
QueryExecutionErrors.malformedRecordsDetectedInRecordParsingError(
+input.toString(),
+new BadRecordException(() -> input, () -> new InternalRow[0],
throwable));
+ }
+ throw new Error(throwable);
Review Comment:
Sorry I missed the `NonFatal` part. Then using Scala is better as it's a bit
weird to simulate NonFatal in Java.
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
panbingkun commented on code in PR #45714:
URL: https://github.com/apache/spark/pull/45714#discussion_r1542275472
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/variant/VariantExpressionEvalUtils.scala:
##
@@ -0,0 +1,44 @@
+/*
+ * 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.spark.sql.catalyst.expressions.variant
+
+import scala.util.control.NonFatal
+
+import org.apache.spark.sql.catalyst.util.BadRecordException
+import org.apache.spark.sql.errors.QueryExecutionErrors
+import org.apache.spark.types.variant.{VariantBuilder,
VariantSizeLimitException, VariantUtil}
+import org.apache.spark.unsafe.types.{UTF8String, VariantVal}
+
+/**
+ * A utility class for constructing variant expressions.
+ */
+object VariantExpressionEvalUtils {
+
+ def parseJson(input: UTF8String): VariantVal = {
+try {
+ val v = VariantBuilder.parseJson(input.toString)
+ new VariantVal(v.getValue, v.getMetadata)
+} catch {
+ case _: VariantSizeLimitException =>
+throw
QueryExecutionErrors.variantSizeLimitError(VariantUtil.SIZE_LIMIT, "parse_json")
+ case NonFatal(e) =>
+throw
QueryExecutionErrors.malformedRecordsDetectedInRecordParsingError(
Review Comment:
> can we make these two methods return `SparkRuntimeException`? Then we can
write the code in Java.
Done
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
panbingkun commented on code in PR #45714:
URL: https://github.com/apache/spark/pull/45714#discussion_r1542275365
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/variant/VariantExpressionEvalUtils.scala:
##
@@ -0,0 +1,44 @@
+/*
+ * 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.spark.sql.catalyst.expressions.variant
+
+import scala.util.control.NonFatal
+
+import org.apache.spark.sql.catalyst.util.BadRecordException
+import org.apache.spark.sql.errors.QueryExecutionErrors
+import org.apache.spark.types.variant.{VariantBuilder,
VariantSizeLimitException, VariantUtil}
+import org.apache.spark.unsafe.types.{UTF8String, VariantVal}
+
+/**
+ * A utility class for constructing variant expressions.
+ */
+object VariantExpressionEvalUtils {
+
+ def parseJson(input: UTF8String): VariantVal = {
+try {
+ val v = VariantBuilder.parseJson(input.toString)
+ new VariantVal(v.getValue, v.getMetadata)
+} catch {
+ case _: VariantSizeLimitException =>
+throw
QueryExecutionErrors.variantSizeLimitError(VariantUtil.SIZE_LIMIT, "parse_json")
+ case NonFatal(e) =>
+throw
QueryExecutionErrors.malformedRecordsDetectedInRecordParsingError(
Review Comment:
The scala version of `VariantExpressionEvalUtils` seen in `decompilation`,
as above.
##
sql/core/src/test/scala/org/apache/spark/sql/VariantFunctionSuite.scala:
##
@@ -0,0 +1,84 @@
+/*
+ * 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.spark.sql
+
+import org.apache.spark.sql.catalyst.expressions.{CreateArray,
CreateNamedStruct, Literal, StructsToJson}
+import org.apache.spark.sql.catalyst.expressions.variant.ParseJson
+import org.apache.spark.sql.execution.WholeStageCodegenExec
+import org.apache.spark.sql.test.SharedSparkSession
+import org.apache.spark.types.variant.VariantBuilder
+import org.apache.spark.unsafe.types.VariantVal
+
+class VariantFunctionSuite extends QueryTest with SharedSparkSession {
Review Comment:
Done
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
panbingkun commented on code in PR #45714:
URL: https://github.com/apache/spark/pull/45714#discussion_r1542274672
##
sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/variant/VariantExpressionEvalUtils.java:
##
@@ -0,0 +1,52 @@
+/*
+ * 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.spark.sql.catalyst.expressions.variant;
+
+import scala.util.control.NonFatal;
+
+import org.apache.spark.sql.catalyst.InternalRow;
+import org.apache.spark.sql.catalyst.util.BadRecordException;
+import org.apache.spark.sql.errors.QueryExecutionErrors;
+import org.apache.spark.types.variant.Variant;
+import org.apache.spark.types.variant.VariantUtil;
+import org.apache.spark.types.variant.VariantBuilder;
+import org.apache.spark.types.variant.VariantSizeLimitException;
+import org.apache.spark.unsafe.types.UTF8String;
+import org.apache.spark.unsafe.types.VariantVal;
+
+/**
+ * A utility class for constructing variant expressions.
+ */
+public class VariantExpressionEvalUtils {
+
+ public static VariantVal parseJson(UTF8String input) {
+try {
+ Variant v = VariantBuilder.parseJson(input.toString());
+ return new VariantVal(v.getValue(), v.getMetadata());
+} catch (VariantSizeLimitException e) {
+ throw QueryExecutionErrors.variantSizeLimitError(VariantUtil.SIZE_LIMIT,
"parse_json");
+} catch (Throwable throwable) {
+ if (NonFatal.apply(throwable)) {
+throw
QueryExecutionErrors.malformedRecordsDetectedInRecordParsingError(
+input.toString(),
+new BadRecordException(() -> input, () -> new InternalRow[0],
throwable));
+ }
+ throw new Error(throwable);
Review Comment:
Although there are some `differences` in the logic seen when `decompiling`
Scala, I think `throwing an Error` is a good compromise.
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
panbingkun commented on code in PR #45714:
URL: https://github.com/apache/spark/pull/45714#discussion_r1542242532
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/variant/VariantExpressionEvalUtils.scala:
##
@@ -0,0 +1,44 @@
+/*
+ * 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.spark.sql.catalyst.expressions.variant
+
+import scala.util.control.NonFatal
+
+import org.apache.spark.sql.catalyst.util.BadRecordException
+import org.apache.spark.sql.errors.QueryExecutionErrors
+import org.apache.spark.types.variant.{VariantBuilder,
VariantSizeLimitException, VariantUtil}
+import org.apache.spark.unsafe.types.{UTF8String, VariantVal}
+
+/**
+ * A utility class for constructing variant expressions.
+ */
+object VariantExpressionEvalUtils {
+
+ def parseJson(input: UTF8String): VariantVal = {
+try {
+ val v = VariantBuilder.parseJson(input.toString)
+ new VariantVal(v.getValue, v.getMetadata)
+} catch {
+ case _: VariantSizeLimitException =>
+throw
QueryExecutionErrors.variantSizeLimitError(VariantUtil.SIZE_LIMIT, "parse_json")
+ case NonFatal(e) =>
+throw
QueryExecutionErrors.malformedRecordsDetectedInRecordParsingError(
Review Comment:
https://github.com/apache/spark/assets/15246973/7eef5c95-e6a1-4f02-90db-05526c68b9fe";>
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
cloud-fan commented on code in PR #45714:
URL: https://github.com/apache/spark/pull/45714#discussion_r1541227779
##
sql/core/src/test/scala/org/apache/spark/sql/VariantFunctionSuite.scala:
##
@@ -0,0 +1,84 @@
+/*
+ * 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.spark.sql
+
+import org.apache.spark.sql.catalyst.expressions.{CreateArray,
CreateNamedStruct, Literal, StructsToJson}
+import org.apache.spark.sql.catalyst.expressions.variant.ParseJson
+import org.apache.spark.sql.execution.WholeStageCodegenExec
+import org.apache.spark.sql.test.SharedSparkSession
+import org.apache.spark.types.variant.VariantBuilder
+import org.apache.spark.unsafe.types.VariantVal
+
+class VariantFunctionSuite extends QueryTest with SharedSparkSession {
Review Comment:
how about `VariantEndToEndSuite`?
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
cloud-fan commented on code in PR #45714:
URL: https://github.com/apache/spark/pull/45714#discussion_r1541224520
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/variant/VariantExpressionEvalUtils.scala:
##
@@ -0,0 +1,44 @@
+/*
+ * 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.spark.sql.catalyst.expressions.variant
+
+import scala.util.control.NonFatal
+
+import org.apache.spark.sql.catalyst.util.BadRecordException
+import org.apache.spark.sql.errors.QueryExecutionErrors
+import org.apache.spark.types.variant.{VariantBuilder,
VariantSizeLimitException, VariantUtil}
+import org.apache.spark.unsafe.types.{UTF8String, VariantVal}
+
+/**
+ * A utility class for constructing variant expressions.
+ */
+object VariantExpressionEvalUtils {
+
+ def parseJson(input: UTF8String): VariantVal = {
+try {
+ val v = VariantBuilder.parseJson(input.toString)
+ new VariantVal(v.getValue, v.getMetadata)
+} catch {
+ case _: VariantSizeLimitException =>
+throw
QueryExecutionErrors.variantSizeLimitError(VariantUtil.SIZE_LIMIT, "parse_json")
+ case NonFatal(e) =>
+throw
QueryExecutionErrors.malformedRecordsDetectedInRecordParsingError(
Review Comment:
can we make these two methods return `SparkRuntimeException`? Then we can
write the code in Java.
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
panbingkun commented on code in PR #45714:
URL: https://github.com/apache/spark/pull/45714#discussion_r1540969833
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/variant/VariantEvaluator.scala:
##
@@ -0,0 +1,42 @@
+/*
+ * 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.spark.sql.catalyst.expressions.variant
+
+import scala.util.control.NonFatal
+
+import org.apache.spark.sql.catalyst.util.BadRecordException
+import org.apache.spark.sql.errors.QueryExecutionErrors
+import org.apache.spark.types.variant.{VariantBuilder,
VariantSizeLimitException, VariantUtil}
+import org.apache.spark.unsafe.types.{UTF8String, VariantVal}
+
+object VariantEvaluator {
+
+ def evaluate(input: UTF8String): VariantVal = {
+if (input == null) return null
Review Comment:
Yeah, you are correct!
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/variant/variantExpressions.scala:
##
@@ -39,27 +33,22 @@ import org.apache.spark.unsafe.types._
group = "variant_funcs"
)
// scalastyle:on line.size.limit
-case class ParseJson(child: Expression) extends UnaryExpression
- with NullIntolerant with ExpectsInputTypes with CodegenFallback {
+case class ParseJson(child: Expression)
+ extends UnaryExpression with RuntimeReplaceable with ImplicitCastInputTypes {
+
+ override lazy val replacement: Expression = StaticInvoke(
+VariantEvaluator.getClass,
+VariantType,
+"evaluate",
Review Comment:
Done
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
panbingkun commented on code in PR #45714:
URL: https://github.com/apache/spark/pull/45714#discussion_r1540969225
##
sql/core/src/test/scala/org/apache/spark/sql/VariantFunctionSuite.scala:
##
@@ -0,0 +1,188 @@
+/*
+ * 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.spark.sql
+
+import org.apache.spark.SparkThrowable
+import org.apache.spark.sql.catalyst.expressions.{CreateArray,
CreateNamedStruct, Literal, StructsToJson}
+import org.apache.spark.sql.catalyst.expressions.variant.ParseJson
+import org.apache.spark.sql.execution.WholeStageCodegenExec
+import org.apache.spark.sql.test.SharedSparkSession
+import org.apache.spark.types.variant.VariantBuilder
+import org.apache.spark.types.variant.VariantUtil._
+import org.apache.spark.unsafe.types.VariantVal
+
+class VariantFunctionSuite extends QueryTest with SharedSparkSession {
+ import testImplicits._
+
+ test("parse_json") {
+def check(json: String, expectedValue: Array[Byte], expectedMetadata:
Array[Byte]): Unit = {
+ val df = Seq(json).toDF("v")
+ val variantDF = df.select(Column(ParseJson(Column("v").expr)))
+ val expected = new VariantVal(expectedValue, expectedMetadata)
+ checkAnswer(variantDF, Seq(Row(expected)))
+}
+
Review Comment:
I put the `type coercion` and `negative` of parseJson into the UT
`VariantExpressionEvalUtilsSuite`.
The case involving `StructsToJson` be placed in UT `VariantFunctionSuite`,
as it may be more suitable for testing in end-to-end scenarios.
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
panbingkun commented on code in PR #45714:
URL: https://github.com/apache/spark/pull/45714#discussion_r1540912401
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/variant/VariantEvaluator.scala:
##
@@ -0,0 +1,42 @@
+/*
+ * 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.spark.sql.catalyst.expressions.variant
+
+import scala.util.control.NonFatal
+
+import org.apache.spark.sql.catalyst.util.BadRecordException
+import org.apache.spark.sql.errors.QueryExecutionErrors
+import org.apache.spark.types.variant.{VariantBuilder,
VariantSizeLimitException, VariantUtil}
+import org.apache.spark.unsafe.types.{UTF8String, VariantVal}
+
+object VariantEvaluator {
Review Comment:
> Can we write it in Java?
- If we write it in `Java`, we may have to write it like this:
https://github.com/apache/spark/assets/15246973/077fdf50-d07b-4a60-b20a-e7cd7078a5ae";>
- Otherwise, it's like this:
https://github.com/apache/spark/assets/15246973/4d76a8ac-bebe-491e-9997-17ab49922cc6";>
- This will result in compile errors, as shown below:
https://github.com/apache/spark/assets/15246973/caa68fe5-1950-4f46-8f3f-d1717580622e";>
https://github.com/apache/spark/assets/15246973/1568f8a2-67b0-4a8a-8003-74c6b7704066";>
Because the class `StaticInvoke` did not consider that the method may
`throw an exception` when codegen.
https://github.com/apache/spark/blob/87449c3f1d65a430fec60981e364e552165fa075/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala#L322-L340
(PS: Maybe we can use a separate PR to improve `StaticInvoke` later?)
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
panbingkun commented on code in PR #45714:
URL: https://github.com/apache/spark/pull/45714#discussion_r1540605287
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/variant/VariantEvaluator.scala:
##
@@ -0,0 +1,42 @@
+/*
+ * 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.spark.sql.catalyst.expressions.variant
+
+import scala.util.control.NonFatal
+
+import org.apache.spark.sql.catalyst.util.BadRecordException
+import org.apache.spark.sql.errors.QueryExecutionErrors
+import org.apache.spark.types.variant.{VariantBuilder,
VariantSizeLimitException, VariantUtil}
+import org.apache.spark.unsafe.types.{UTF8String, VariantVal}
+
+object VariantEvaluator {
Review Comment:
Let me try.
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
panbingkun commented on code in PR #45714:
URL: https://github.com/apache/spark/pull/45714#discussion_r1540604479
##
sql/core/src/test/scala/org/apache/spark/sql/VariantFunctionSuite.scala:
##
@@ -0,0 +1,188 @@
+/*
+ * 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.spark.sql
+
+import org.apache.spark.SparkThrowable
+import org.apache.spark.sql.catalyst.expressions.{CreateArray,
CreateNamedStruct, Literal, StructsToJson}
+import org.apache.spark.sql.catalyst.expressions.variant.ParseJson
+import org.apache.spark.sql.execution.WholeStageCodegenExec
+import org.apache.spark.sql.test.SharedSparkSession
+import org.apache.spark.types.variant.VariantBuilder
+import org.apache.spark.types.variant.VariantUtil._
+import org.apache.spark.unsafe.types.VariantVal
+
+class VariantFunctionSuite extends QueryTest with SharedSparkSession {
+ import testImplicits._
+
+ test("parse_json") {
+def check(json: String, expectedValue: Array[Byte], expectedMetadata:
Array[Byte]): Unit = {
+ val df = Seq(json).toDF("v")
+ val variantDF = df.select(Column(ParseJson(Column("v").expr)))
+ val expected = new VariantVal(expectedValue, expectedMetadata)
+ checkAnswer(variantDF, Seq(Row(expected)))
+}
+
Review Comment:
Yeah, okay.
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
panbingkun commented on code in PR #45714:
URL: https://github.com/apache/spark/pull/45714#discussion_r1540603335
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/variant/variantExpressions.scala:
##
@@ -39,27 +33,22 @@ import org.apache.spark.unsafe.types._
group = "variant_funcs"
)
// scalastyle:on line.size.limit
-case class ParseJson(child: Expression) extends UnaryExpression
- with NullIntolerant with ExpectsInputTypes with CodegenFallback {
+case class ParseJson(child: Expression)
+ extends UnaryExpression with RuntimeReplaceable with ImplicitCastInputTypes {
Review Comment:
Okay, let me update it.
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
cloud-fan commented on code in PR #45714:
URL: https://github.com/apache/spark/pull/45714#discussion_r1540599398
##
sql/core/src/test/scala/org/apache/spark/sql/VariantFunctionSuite.scala:
##
@@ -0,0 +1,188 @@
+/*
+ * 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.spark.sql
+
+import org.apache.spark.SparkThrowable
+import org.apache.spark.sql.catalyst.expressions.{CreateArray,
CreateNamedStruct, Literal, StructsToJson}
+import org.apache.spark.sql.catalyst.expressions.variant.ParseJson
+import org.apache.spark.sql.execution.WholeStageCodegenExec
+import org.apache.spark.sql.test.SharedSparkSession
+import org.apache.spark.types.variant.VariantBuilder
+import org.apache.spark.types.variant.VariantUtil._
+import org.apache.spark.unsafe.types.VariantVal
+
+class VariantFunctionSuite extends QueryTest with SharedSparkSession {
+ import testImplicits._
+
+ test("parse_json") {
+def check(json: String, expectedValue: Array[Byte], expectedMetadata:
Array[Byte]): Unit = {
+ val df = Seq(json).toDF("v")
+ val variantDF = df.select(Column(ParseJson(Column("v").expr)))
+ val expected = new VariantVal(expectedValue, expectedMetadata)
+ checkAnswer(variantDF, Seq(Row(expected)))
+}
+
Review Comment:
They are slightly different. The previous tests are unit tests, using
`checkEvaluation`. The new tests here are end-to-end tests, which are slower to
run.
I'd suggest we test the implementation java function directly.
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
cloud-fan commented on code in PR #45714:
URL: https://github.com/apache/spark/pull/45714#discussion_r1540597468
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/variant/variantExpressions.scala:
##
@@ -39,27 +33,22 @@ import org.apache.spark.unsafe.types._
group = "variant_funcs"
)
// scalastyle:on line.size.limit
-case class ParseJson(child: Expression) extends UnaryExpression
- with NullIntolerant with ExpectsInputTypes with CodegenFallback {
+case class ParseJson(child: Expression)
+ extends UnaryExpression with RuntimeReplaceable with ImplicitCastInputTypes {
Review Comment:
let's not change the behavior in this PR. We should still extend
`ExpectsInputTypes`
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
cloud-fan commented on code in PR #45714:
URL: https://github.com/apache/spark/pull/45714#discussion_r1540596819
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/variant/VariantEvaluator.scala:
##
@@ -0,0 +1,42 @@
+/*
+ * 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.spark.sql.catalyst.expressions.variant
+
+import scala.util.control.NonFatal
+
+import org.apache.spark.sql.catalyst.util.BadRecordException
+import org.apache.spark.sql.errors.QueryExecutionErrors
+import org.apache.spark.types.variant.{VariantBuilder,
VariantSizeLimitException, VariantUtil}
+import org.apache.spark.unsafe.types.{UTF8String, VariantVal}
+
+object VariantEvaluator {
Review Comment:
and `VariantExpressionEvalUtils` sounds a better name, if we keep using to
implement variant related expressions.
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
cloud-fan commented on code in PR #45714:
URL: https://github.com/apache/spark/pull/45714#discussion_r1540596093
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/variant/variantExpressions.scala:
##
@@ -39,27 +33,22 @@ import org.apache.spark.unsafe.types._
group = "variant_funcs"
)
// scalastyle:on line.size.limit
-case class ParseJson(child: Expression) extends UnaryExpression
- with NullIntolerant with ExpectsInputTypes with CodegenFallback {
+case class ParseJson(child: Expression)
+ extends UnaryExpression with RuntimeReplaceable with ImplicitCastInputTypes {
+
+ override lazy val replacement: Expression = StaticInvoke(
+VariantEvaluator.getClass,
+VariantType,
+"evaluate",
Review Comment:
The name should match the expression. how about `parseJson`?
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
cloud-fan commented on code in PR #45714:
URL: https://github.com/apache/spark/pull/45714#discussion_r1540595591
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/variant/VariantEvaluator.scala:
##
@@ -0,0 +1,42 @@
+/*
+ * 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.spark.sql.catalyst.expressions.variant
+
+import scala.util.control.NonFatal
+
+import org.apache.spark.sql.catalyst.util.BadRecordException
+import org.apache.spark.sql.errors.QueryExecutionErrors
+import org.apache.spark.types.variant.{VariantBuilder,
VariantSizeLimitException, VariantUtil}
+import org.apache.spark.unsafe.types.{UTF8String, VariantVal}
+
+object VariantEvaluator {
+
+ def evaluate(input: UTF8String): VariantVal = {
+if (input == null) return null
Review Comment:
I believe StaticInvoke returns null for null input automatically.
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
cloud-fan commented on code in PR #45714:
URL: https://github.com/apache/spark/pull/45714#discussion_r1540595249
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/variant/VariantEvaluator.scala:
##
@@ -0,0 +1,42 @@
+/*
+ * 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.spark.sql.catalyst.expressions.variant
+
+import scala.util.control.NonFatal
+
+import org.apache.spark.sql.catalyst.util.BadRecordException
+import org.apache.spark.sql.errors.QueryExecutionErrors
+import org.apache.spark.types.variant.{VariantBuilder,
VariantSizeLimitException, VariantUtil}
+import org.apache.spark.unsafe.types.{UTF8String, VariantVal}
+
+object VariantEvaluator {
Review Comment:
Can we write it in Java?
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
panbingkun commented on code in PR #45714:
URL: https://github.com/apache/spark/pull/45714#discussion_r1540535392
##
sql/core/src/test/scala/org/apache/spark/sql/VariantFunctionSuite.scala:
##
@@ -0,0 +1,188 @@
+/*
+ * 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.spark.sql
+
+import org.apache.spark.SparkThrowable
+import org.apache.spark.sql.catalyst.expressions.{CreateArray,
CreateNamedStruct, Literal, StructsToJson}
+import org.apache.spark.sql.catalyst.expressions.variant.ParseJson
+import org.apache.spark.sql.execution.WholeStageCodegenExec
+import org.apache.spark.sql.test.SharedSparkSession
+import org.apache.spark.types.variant.VariantBuilder
+import org.apache.spark.types.variant.VariantUtil._
+import org.apache.spark.unsafe.types.VariantVal
+
+class VariantFunctionSuite extends QueryTest with SharedSparkSession {
+ import testImplicits._
+
+ test("parse_json") {
+def check(json: String, expectedValue: Array[Byte], expectedMetadata:
Array[Byte]): Unit = {
+ val df = Seq(json).toDF("v")
+ val variantDF = df.select(Column(ParseJson(Column("v").expr)))
+ val expected = new VariantVal(expectedValue, expectedMetadata)
+ checkAnswer(variantDF, Seq(Row(expected)))
+}
+
Review Comment:
The test data is completely one-to-one corresponding to the original data in
`VariantExpressionSuite`.
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
panbingkun commented on code in PR #45714:
URL: https://github.com/apache/spark/pull/45714#discussion_r1540534183
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/variant/variantExpressions.scala:
##
@@ -40,7 +40,7 @@ import org.apache.spark.unsafe.types._
)
// scalastyle:on line.size.limit
case class ParseJson(child: Expression) extends UnaryExpression
- with NullIntolerant with ExpectsInputTypes with CodegenFallback {
+ with NullIntolerant with ExpectsInputTypes {
Review Comment:
The relevant test data involved in the class `VariantExpressionSuite`
through by `evaluate it directly`
has been migrated to the class `VariantFunctionsSuite` through by `test type
coercion`.
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
panbingkun commented on code in PR #45714:
URL: https://github.com/apache/spark/pull/45714#discussion_r1539244780
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/variant/variantExpressions.scala:
##
@@ -40,7 +40,7 @@ import org.apache.spark.unsafe.types._
)
// scalastyle:on line.size.limit
case class ParseJson(child: Expression) extends UnaryExpression
- with NullIntolerant with ExpectsInputTypes with CodegenFallback {
+ with NullIntolerant with ExpectsInputTypes {
Review Comment:
Okay, thank you for your very detailed explanation, let me improve it.
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
cloud-fan commented on code in PR #45714:
URL: https://github.com/apache/spark/pull/45714#discussion_r1539219005
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/variant/variantExpressions.scala:
##
@@ -40,7 +40,7 @@ import org.apache.spark.unsafe.types._
)
// scalastyle:on line.size.limit
case class ParseJson(child: Expression) extends UnaryExpression
- with NullIntolerant with ExpectsInputTypes with CodegenFallback {
+ with NullIntolerant with ExpectsInputTypes {
Review Comment:
For RuntimeReplaceable expression, the test should not evaluate it directly,
but evaluate its replacement. If the replacement is `StaticInvoke`, no
expression-level UT is needed (we should still test type coercion), and just
test the target java method.
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
panbingkun commented on code in PR #45714:
URL: https://github.com/apache/spark/pull/45714#discussion_r1538850826
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/variant/variantExpressions.scala:
##
@@ -40,7 +40,7 @@ import org.apache.spark.unsafe.types._
)
// scalastyle:on line.size.limit
case class ParseJson(child: Expression) extends UnaryExpression
- with NullIntolerant with ExpectsInputTypes with CodegenFallback {
+ with NullIntolerant with ExpectsInputTypes {
Review Comment:
It seems not possible. Currently, some of UT `conflict` with it, such as
https://github.com/apache/spark/assets/15246973/37bfb6e6-110e-4dcc-96bb-10d13eb2dfcd";>
https://github.com/apache/spark/blob/becbf8b942132b82e7b906c63ea6077649329b93/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/variant/VariantExpressionSuite.scala#L39
https://github.com/apache/spark/blob/becbf8b942132b82e7b906c63ea6077649329b93/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala#L87
https://github.com/apache/spark/blob/becbf8b942132b82e7b906c63ea6077649329b93/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala#L256
https://github.com/apache/spark/blob/becbf8b942132b82e7b906c63ea6077649329b93/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala#L248
https://github.com/apache/spark/blob/becbf8b942132b82e7b906c63ea6077649329b93/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Expression.scala#L403-L415
https://github.com/apache/spark/assets/15246973/74e93b90-2238-4da3-8ed5-94583be2d0e3";>
If we `inherit with` `RuntimeReplaceable `, we cannot `override` the method
`def eval(input: InternalRow = null): Any`, which will lead to the above error.
Should we remove the related UT (VariantExpressionSuite#[`parse_json`,
`parse_json negative`, `round-trip`, `to_json with nested variant`])?
In addition, `the newly added UT` in this pr is ok.
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
panbingkun commented on code in PR #45714:
URL: https://github.com/apache/spark/pull/45714#discussion_r1538850826
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/variant/variantExpressions.scala:
##
@@ -40,7 +40,7 @@ import org.apache.spark.unsafe.types._
)
// scalastyle:on line.size.limit
case class ParseJson(child: Expression) extends UnaryExpression
- with NullIntolerant with ExpectsInputTypes with CodegenFallback {
+ with NullIntolerant with ExpectsInputTypes {
Review Comment:
It seems not possible. Currently, some of UT `conflict` with it, such as
https://github.com/apache/spark/assets/15246973/37bfb6e6-110e-4dcc-96bb-10d13eb2dfcd";>
https://github.com/apache/spark/blob/becbf8b942132b82e7b906c63ea6077649329b93/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/variant/VariantExpressionSuite.scala#L39
https://github.com/apache/spark/blob/becbf8b942132b82e7b906c63ea6077649329b93/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala#L87
https://github.com/apache/spark/blob/becbf8b942132b82e7b906c63ea6077649329b93/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala#L256
https://github.com/apache/spark/blob/becbf8b942132b82e7b906c63ea6077649329b93/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala#L248
https://github.com/apache/spark/blob/becbf8b942132b82e7b906c63ea6077649329b93/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Expression.scala#L403-L415
https://github.com/apache/spark/assets/15246973/74e93b90-2238-4da3-8ed5-94583be2d0e3";>
If we inherit `RuntimeReplaceable `, we cannot overload the method `def
eval(input: InternalRow = null): Any`, which will lead to the above error.
Should we remove the related UT (VariantExpressionSuite#[`parse_json`,
`parse_json negative`, `round-trip`, `to_json with nested variant`])?
In addition, `the newly added UT` in this pr is ok.
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
panbingkun commented on code in PR #45714:
URL: https://github.com/apache/spark/pull/45714#discussion_r1538850826
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/variant/variantExpressions.scala:
##
@@ -40,7 +40,7 @@ import org.apache.spark.unsafe.types._
)
// scalastyle:on line.size.limit
case class ParseJson(child: Expression) extends UnaryExpression
- with NullIntolerant with ExpectsInputTypes with CodegenFallback {
+ with NullIntolerant with ExpectsInputTypes {
Review Comment:
It seems not possible. Currently, some of UT `conflict` with it, such as
https://github.com/apache/spark/assets/15246973/37bfb6e6-110e-4dcc-96bb-10d13eb2dfcd";>
https://github.com/apache/spark/blob/becbf8b942132b82e7b906c63ea6077649329b93/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/variant/VariantExpressionSuite.scala#L39
https://github.com/apache/spark/blob/becbf8b942132b82e7b906c63ea6077649329b93/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala#L87
https://github.com/apache/spark/blob/becbf8b942132b82e7b906c63ea6077649329b93/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala#L256
https://github.com/apache/spark/blob/becbf8b942132b82e7b906c63ea6077649329b93/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ExpressionEvalHelper.scala#L248
https://github.com/apache/spark/blob/becbf8b942132b82e7b906c63ea6077649329b93/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Expression.scala#L403-L415
If we inherit `RuntimeReplaceable `, we cannot overload the method `def
eval(input: InternalRow = null): Any`, which will lead to the above error.
Should we remove the related UT (VariantExpressionSuite#[`parse_json`,
`parse_json negative`, `round-trip`, `to_json with nested variant`])?
In addition, `the newly added UT` in this pr is ok.
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
panbingkun commented on code in PR #45714:
URL: https://github.com/apache/spark/pull/45714#discussion_r1538681477
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/variant/variantExpressions.scala:
##
@@ -40,7 +40,7 @@ import org.apache.spark.unsafe.types._
)
// scalastyle:on line.size.limit
case class ParseJson(child: Expression) extends UnaryExpression
- with NullIntolerant with ExpectsInputTypes with CodegenFallback {
+ with NullIntolerant with ExpectsInputTypes {
Review Comment:
I'll try it.
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
cloud-fan commented on code in PR #45714:
URL: https://github.com/apache/spark/pull/45714#discussion_r1538680755
##
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/variant/variantExpressions.scala:
##
@@ -40,7 +40,7 @@ import org.apache.spark.unsafe.types._
)
// scalastyle:on line.size.limit
case class ParseJson(child: Expression) extends UnaryExpression
- with NullIntolerant with ExpectsInputTypes with CodegenFallback {
+ with NullIntolerant with ExpectsInputTypes {
Review Comment:
Can we use `RuntimeReplaceable` with `StaticInvoke` to implement codegen?
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
panbingkun commented on PR #45714: URL: https://github.com/apache/spark/pull/45714#issuecomment-2019522184 cc @cloud-fan @MaxGekk -- 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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
