-
Notifications
You must be signed in to change notification settings - Fork 13.7k
[FLINK-37923][sql] Introduce VARIANT type and PARSE_JSON to Flink SQL #26655
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work @Sxnan! I left a couple of comments that should improve consistency when integrating this type into the existing type system.
flink-core/src/main/java/org/apache/flink/types/variant/BinaryVariant.java
Outdated
Show resolved
Hide resolved
flink-core/src/main/java/org/apache/flink/types/variant/BinaryVariant.java
Outdated
Show resolved
Hide resolved
flink-core/src/main/java/org/apache/flink/types/variant/BinaryVariant.java
Outdated
Show resolved
Hide resolved
flink-core/src/main/java/org/apache/flink/types/variant/BinaryVariantUtil.java
Outdated
Show resolved
Hide resolved
flink-core/src/main/java/org/apache/flink/types/variant/BinaryVariantUtil.java
Outdated
Show resolved
Hide resolved
...-planner/src/main/scala/org/apache/flink/table/planner/codegen/calls/FunctionGenerator.scala
Outdated
Show resolved
Hide resolved
...anner/src/test/java/org/apache/flink/table/planner/runtime/stream/sql/VariantJavaITCase.java
Outdated
Show resolved
Hide resolved
...anner/src/test/scala/org/apache/flink/table/planner/runtime/stream/sql/AggregateITCase.scala
Outdated
Show resolved
Hide resolved
...er/src/test/scala/org/apache/flink/table/planner/runtime/stream/sql/VariantScalaITCase.scala
Outdated
Show resolved
Hide resolved
...flink-table-runtime/src/main/java/org/apache/flink/table/runtime/functions/VariantUtils.java
Outdated
Show resolved
Hide resolved
316415e
to
e2adf87
Compare
@twalthr Thanks for the detailed review! I updated the PR accordingly, and all the comments should be addressed. Please take another look. |
Thanks @Sxnan. I added it for my list for tomorrow. I'm sure it can still make it before the feature freeze. |
Hi @twalthr, could you take another look at your earliest convenience, in case we need to make some final adjustment to the PR before the feature freeze tomorrow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @Sxnan. I think the PR should definitely make it to 2.1 release. I added my last set of comments.
flink-core/src/main/java/org/apache/flink/types/variant/BinaryVariantUtil.java
Outdated
Show resolved
Hide resolved
flink-core/src/test/java/org/apache/flink/api/common/typeutils/base/VariantSerializerTest.java
Show resolved
Hide resolved
flink-table/flink-table-common/src/main/java/org/apache/flink/table/api/DataTypes.java
Show resolved
Hide resolved
|
||
/** Variant represent a semi-structured data. */ | ||
@PublicEvolving | ||
public interface Variant extends Serializable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could imagine that users will request it to more easily pass it to constructors of ProcessFunction or having it in member variables of ProcessFunction as well. This is also the reason why Row
is Serializable
. But this can be a followup. Given all the utilities we have, ensuring serializability should not be too difficult to implement with a custom readObject
/writeObject
.
flink-core/src/main/java/org/apache/flink/types/variant/Variant.java
Outdated
Show resolved
Hide resolved
...src/test/java/org/apache/flink/table/planner/plan/nodes/exec/stream/VariantSemanticTest.java
Show resolved
Hide resolved
Hi @twalthr, thanks for the review! I updated the PR accordingly in the last two fixup commits. Please take another look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this great feature @Sxnan!
Fixup commits are squashed. Will merge after the test passes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice feature, @Sxnan 🙂 I somehow didn't spot the docs. Are they covered in another PR?
@gustavodemorais Yes, it will be covered later in another PR. |
What is the purpose of the change
This pull request introduces the Variant data structure to represent semi-structured data, a new SQL type variant, and a builtin method to parse json string to the variant type.
Brief change log
Variant
andBinaryVariant
PARSE_JSON
to Flink SQLVerifying this change
This change added tests and can be verified.
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: yesDocumentation