-
Notifications
You must be signed in to change notification settings - Fork 321
Feat: to_json Infinity/-Infinity Nan values support #3875
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
base: main
Are you sure you want to change the base?
Changes from all commits
768b3e9
c68c342
d887555
231aa90
9500bbb
9577481
3791557
7c2f082
609a605
a151b2c
ad3e7f5
ea92e4b
8dfeca3
559741e
ebda14e
408152e
d7857b2
aef41be
5ac1c58
9ae8e23
5ca3888
160a817
88fc313
e14c180
610a885
f8acb2c
ec94897
43405e4
47b4915
26e2682
6cb5f07
ec194fb
e0a02bf
c322014
256fccb
2dce727
c7bf49f
912c8f9
561a664
931b5d4
14a92f7
d926ef4
671412c
49424d1
57f2ccf
c9f52d1
67f72d9
84e50fa
5284b2a
314e594
3c6a3dd
f73b391
ac8292f
c9c140e
decca58
0919b33
bb78fd5
169fb36
2cf9990
fb5e795
9229213
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -105,57 +105,37 @@ object CometGetArrayStructFields extends CometExpressionSerde[GetArrayStructFiel | |
|
|
||
| object CometStructsToJson extends CometExpressionSerde[StructsToJson] { | ||
|
|
||
| override def getIncompatibleReasons(): Seq[String] = Seq( | ||
| "Does not support `+Infinity` and `-Infinity` for numeric types (float, double)." + | ||
| " (https://github.com/apache/datafusion-comet/issues/3016)") | ||
|
|
||
| override def getSupportLevel(expr: StructsToJson): SupportLevel = | ||
| Incompatible( | ||
| Some( | ||
| "Does not support Infinity/-Infinity for numeric types" + | ||
| " (https://github.com/apache/datafusion-comet/issues/3016)")) | ||
| override def getSupportLevel(expr: StructsToJson): SupportLevel = { | ||
| if (expr.options.nonEmpty) { | ||
| return Unsupported(Some("StructsToJson with options is not supported")) | ||
| } | ||
| val dataType = expr.child.dataType | ||
| if (!isSupportedType(dataType)) { | ||
| return Unsupported(Some(s"Struct type: $dataType contains unsupported types")) | ||
| } | ||
| Compatible() | ||
| } | ||
|
|
||
| override def convert( | ||
| expr: StructsToJson, | ||
| inputs: Seq[Attribute], | ||
| binding: Boolean): Option[ExprOuterClass.Expr] = { | ||
| if (expr.options.nonEmpty) { | ||
| withInfo(expr, "StructsToJson with options is not supported") | ||
| None | ||
| } else { | ||
| val isSupported = expr.child.dataType match { | ||
| case s: StructType => | ||
| s.fields.forall(f => isSupportedType(f.dataType)) | ||
| case _: MapType | _: ArrayType => | ||
| // Spark supports map and array in StructsToJson but this is not yet | ||
| // implemented in Comet | ||
| false | ||
| case _ => | ||
| false | ||
| } | ||
|
|
||
| if (isSupported) { | ||
| exprToProtoInternal(expr.child, inputs, binding) match { | ||
| case Some(p) => | ||
| val toJson = ExprOuterClass.ToJson | ||
| .newBuilder() | ||
| .setChild(p) | ||
| .setTimezone(expr.timeZoneId.getOrElse("UTC")) | ||
| .setIgnoreNullFields(true) | ||
| .build() | ||
| Some( | ||
| ExprOuterClass.Expr | ||
| .newBuilder() | ||
| .setToJson(toJson) | ||
| .build()) | ||
| case _ => | ||
| withInfo(expr, expr.child) | ||
| None | ||
| } | ||
| } else { | ||
| withInfo(expr, "Unsupported data type", expr.child) | ||
| exprToProtoInternal(expr.child, inputs, binding) match { | ||
| case Some(p) => | ||
| val toJson = ExprOuterClass.ToJson | ||
| .newBuilder() | ||
| .setChild(p) | ||
| .setTimezone(expr.timeZoneId.getOrElse("UTC")) | ||
| .setIgnoreNullFields(true) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, the native code seems to ignore this field?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we need to respect |
||
| .build() | ||
| Some( | ||
| ExprOuterClass.Expr | ||
| .newBuilder() | ||
| .setToJson(toJson) | ||
| .build()) | ||
| case _ => | ||
| withInfo(expr, expr.child) | ||
| None | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,14 +16,20 @@ | |
| -- under the License. | ||
|
|
||
| statement | ||
| CREATE TABLE test_to_json(a int, b string) USING parquet | ||
| CREATE TABLE test_to_json(a int, b string, f float, d double) USING parquet | ||
|
|
||
| statement | ||
| INSERT INTO test_to_json VALUES (1, 'hello'), (NULL, NULL), (0, '') | ||
| INSERT INTO test_to_json VALUES (1, 'hello', cast('NaN' as float), cast('Infinity' as double)), (NULL, NULL, NULL, NULL), (0, '', 0.0, 0.0) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you add |
||
|
|
||
| query spark_answer_only | ||
| SELECT to_json(named_struct('a', a, 'b', b)) FROM test_to_json | ||
| query | ||
| SELECT to_json(named_struct('a', a, 'b', b, 'f', f, 'd', d)) FROM test_to_json | ||
|
|
||
| -- literal arguments | ||
| query spark_answer_only | ||
| query | ||
| SELECT to_json(named_struct('a', 1, 'b', 'hello')) | ||
|
|
||
| query expect_fallback(StructsToJson with options is not supported) | ||
| SELECT to_json(named_struct('a', a, 'b', b), map('timestampFormat', 'dd/MM/yyyy')) FROM test_to_json | ||
|
|
||
| query expect_fallback(Struct type: StructType(StructField(a,IntegerType,true),StructField(b,ArrayType(StringType,true),false)) contains unsupported types) | ||
| SELECT to_json(named_struct('a', a, 'b', array(b))) FROM test_to_json | ||
Uh oh!
There was an error while loading. Please reload this page.