You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "panbingkun (via GitHub)" <gi...@apache.org> on 2024/03/26 03:28:16 UTC
[PR] [WIP] Codegen Support for parse_json (variant) [spark]
panbingkun opened a new pull request, #45714:
URL: https://github.com/apache/spark/pull/45714
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'common/utils/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
### Was this patch authored or co-authored using generative AI tooling?
<!--
If generative AI tooling has been used in the process of authoring this patch, please include the
phrase: 'Generated-by: ' followed by the name of the tool and its version.
If no, write 'No'.
Please refer to the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) for details.
-->
--
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@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
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:
<img width="1395" alt="image" src="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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
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:
<img width="751" alt="image" src="https://github.com/apache/spark/assets/15246973/077fdf50-d07b-4a60-b20a-e7cd7078a5ae">
- Otherwise, it's like this:
<img width="760" alt="image" src="https://github.com/apache/spark/assets/15246973/4d76a8ac-bebe-491e-9997-17ab49922cc6">
- This will result in compile errors, as shown below:
<img width="1000" alt="image" src="https://github.com/apache/spark/assets/15246973/caa68fe5-1950-4f46-8f3f-d1717580622e">
<img width="970" alt="image" src="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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
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
<img width="1025" alt="image" src="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
<img width="573" alt="image" src="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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
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
<img width="1025" alt="image" src="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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
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
<img width="1025" alt="image" src="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
<img width="573" alt="image" src="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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
Posted by "panbingkun (via GitHub)" <gi...@apache.org>.
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47559][SQL] Codegen Support for variant `parse_json` [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
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-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org