Skip to content

[SPARK-49939][SQL] Codegen Support for json_object_keys (by Invoke & RuntimeReplaceable) #48428

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
panbingkun wants to merge 3 commits into from
Closed

[SPARK-49939][SQL] Codegen Support for json_object_keys (by Invoke & RuntimeReplaceable) #48428

panbingkun wants to merge 3 commits into from

Conversation

@panbingkun
Copy link
Contributor

@panbingkun panbingkun commented Oct 12, 2024

What changes were proposed in this pull request?

The pr aims to add Codegen Support for json_object_keys.

Why are the changes needed?

  • improve codegen coverage.
  • simplified code.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Pass GA & Existed UT (eg: JsonFunctionsSuite#json_object_keys function)

Was this patch authored or co-authored using generative AI tooling?

No.

@panbingkun panbingkun marked this pull request as ready for review October 12, 2024 16:33
MaxGekk
MaxGekk reviewed Oct 12, 2024
View reviewed changes
...talyst/src/main/java/org/apache/spark/sql/catalyst/expressions/json/JsonExpressionUtils.java
jsonParser.skipChildren();
}
return new GenericArrayData(arrayBufferOfKeys.toArray());
} catch (IOException e) {
Copy link
Member

@MaxGekk MaxGekk Oct 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't you handle JsonProcessingException?

Copy link
Contributor Author

@panbingkun panbingkun Oct 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If written as one of the following two styles:

  • A.
image
  • B.
image

  • Because:
    JsonProcessingException extends JacksonException and JacksonException extends java.io.IOException

  • So, Based on my knowledge that catch java.io.IOException should already be enough to achieve the goal.

@MaxGekk
Copy link
Member

MaxGekk commented Oct 13, 2024

+1, LGTM. Merging to master.
Thank you, @panbingkun.

@MaxGekk MaxGekk closed this in 54fd408 Oct 13, 2024
@SubhamSinghal
Copy link

SubhamSinghal commented Oct 14, 2024

@panbingkun Do you plan to add codeGen support for StructsToJson and JsonToStructs expression as well?

@panbingkun
Copy link
Contributor Author

panbingkun commented Oct 14, 2024

JsonToStructs

Yes!

@SubhamSinghal
Copy link

SubhamSinghal commented Oct 14, 2024

cool. If you are not planning for StructsToJson, then I will take your code as ref and try to implement it.

@panbingkun
Copy link
Contributor Author

panbingkun commented Oct 15, 2024

cool. If you are not planning for StructsToJson, then I will take your code as ref and try to implement it.

I don't think StructsToJson(to_json) is suitable for using Invoke & RuntimeReplaceable to support codegen.
A draft implementation can be found at: #48467

@SubhamSinghal
Copy link

SubhamSinghal commented Oct 15, 2024 •
edited
Loading

sorry for naive question, any reason why to_json is not suitable for using Invoke & RuntimeReplaceable? I was checking proto equivalent function toProtobuf expression which extends RuntimeReplaceable

@panbingkun
Copy link
Contributor Author

panbingkun commented Oct 15, 2024

sorry for naive question, any reason why to_json is not suitable for using Invoke & RuntimeReplaceable? I was checking proto equivalent function toProtobuf expression which extends RuntimeReplaceable

If we use Invoke and RuntimeReplaceable to support Codegen for to_json,

  • Do we need to create a JacksonGenerator every time? Based on my understanding, if we do this, the performance doesn't seem to be optimal.
  • If we only create JacksonGenerator once in to_json, then we have to pass it in the Invoke call, and we must make JacksonGenerator extend Serializable, which will cause more or less changes to the old logic. Variables that may be used in Invoke, such as writer and converter, all have similar issues.

@panbingkun panbingkun mentioned this pull request Oct 30, 2024
Closed
MaxGekk pushed a commit that referenced this pull request Oct 30, 2024
@panbingkun @MaxGekk
[SPARK-49766][SPARK-49939][SPARK-49967][SPARK-49966][SQL][FOLLOWUP] R…
214f958 
…emove redundant whether is null check

### What changes were proposed in this pull request?
- The pr aims to remove redundant `whether is null check` when implementing codegen using `Invoke` and `StaticInvoke` (they have a `propagateNull` tag, with a default value of `true`)
- The pr is following up:
  #48224
  #48428
  #48467
  #48509

### Why are the changes needed?
Delete unnecessary logic, improve performance, and reduce misunderstandings.

### Does this PR introduce _any_ user-facing change?
No.

### How was this patch tested?
- Pass GA.
- Manually check
```
./build/sbt "sql/testOnly org.apache.spark.sql.JsonFunctionsSuite"
./build/sbt "catalyst/testOnly org.apache.spark.sql.catalyst.expressions.JsonExpressionsSuite"
```
- json_array_length
  JsonFunctionsSuite#`json_array_length function`

- json_object_keys
  JsonFunctionsSuite#`json_object_keys function`

- to_json
 JsonFunctionsSuite#`roundtrip in to_json and from_json - struct`
 JsonFunctionsSuite#`roundtrip in to_json and from_json - array`

- from_json
  JsonFunctionsSuite#`roundtrip in to_json and from_json - struct`

### Was this patch authored or co-authored using generative AI tooling?
No.

Closes #48699 from panbingkun/SPARK-49766_FOLLOWUP.

Authored-by: panbingkun <panbingkun@baidu.com>
Signed-off-by: Max Gekk <max.gekk@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MaxGekk MaxGekk MaxGekk approved these changes

Assignees

No one assigned

Labels

CONNECT SQL

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@panbingkun @MaxGekk @SubhamSinghal