[SPARK-46654][SQL][PYTHON] Make `to_csv` explicitly indicate that it does not support some types of data by panbingkun · Pull Request #44665 · apache/spark

panbingkun · 2024-01-10T13:16:58Z

What changes were proposed in this pull request?

The pr aims to make to_csv explicitly indicate that it does not support some types of data, includes:

Struct
Array
Map
Variant
Binary

Why are the changes needed?

Because the CSV specification does not have standards for these types, and cannot be read back through from_csv.
For this case
Before:

After:

Does this PR introduce any user-facing change?

Yes, to_csv will explicitly indicate that it does not support some types of data.

How was this patch tested?

Add & Update UT.
Pass GA.

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

No.

…etween Regular Spark and Spark Connect

panbingkun · 2024-01-11T02:11:02Z

cc @cloud-fan @LuciferYang

LuciferYang · 2024-01-11T03:16:30Z

@panbingkun Let's re-enable the doctest, this test was pass in pyspark-sql task and failed in pyspark-connect task before

spark/python/pyspark/sql/functions/builtin.py

Lines 15053 to 15058 in 514ecc6

    
               Example 2: Converting a complex StructType to a CSV string 
        
               >>> from pyspark.sql import Row, functions as sf 
        
               >>> data = [(1, Row(age=2, name='Alice', scores=[100, 200, 300]))] 
        
               >>> df = spark.createDataFrame(data, ("key", "value")) 
        
               >>> df.select(sf.to_csv(df.value)).show(truncate=False) # doctest: +SKIP

and remove the TODO

spark/python/pyspark/sql/functions/builtin.py

Lines 15010 to 15013 in 514ecc6

    
           # TODO(SPARK-46654) Re-enable the `Example 2` test after fixing the display 
        
           #  difference between Regular Spark and Spark Connect on `df.show`. 
        
           @_try_remote_functions 
        
           def to_csv(col: "ColumnOrName", options: Optional[Dict[str, str]] = None) -> Column:

also cc @HyukjinKwon

panbingkun · 2024-01-11T03:18:25Z

@panbingkun Let's re-enable the doctest

spark/python/pyspark/sql/functions/builtin.py

Lines 15053 to 15058 in 514ecc6

Example 2: Converting a complex StructType to a CSV string

>>> from pyspark.sql import Row, functions as sf

>>> data = [(1, Row(age=2, name='Alice', scores=[100, 200, 300]))]

>>> df = spark.createDataFrame(data, ("key", "value"))

>>> df.select(sf.to_csv(df.value)).show(truncate=False) # doctest: +SKIP

and remove the TODO

spark/python/pyspark/sql/functions/builtin.py

Lines 15010 to 15013 in 514ecc6

# TODO(SPARK-46654) Re-enable the `Example 2` test after fixing the display

# difference between Regular Spark and Spark Connect on `df.show`.

@_try_remote_functions

def to_csv(col: "ColumnOrName", options: Optional[Dict[str, str]] = None) -> Column:

also cc @HyukjinKwon

Okay, done.

MaxGekk · 2024-01-22T08:29:55Z

-      (row: InternalRow, ordinal: Int) =>
-        row.get(ordinal, dt).toString
+      (row: InternalRow, ordinal: Int) => {
+        val valueUTF8String = ToPrettyString(Literal(row.get(ordinal, dt), dt),


Does ToPrettyString print nested fields in the same way as UnivocityGenerator. For instance, does it follow to datetime patterns specified for the CSV datasource?

Let me think again, maybe we need to rewrite the logic similar to ToPrettyString in UnivocityGenerator based on the characteristics of CSV.

I changed the approach and enabled UnivocityGenerator to support complex type (ArrayType, MapType, StructType) outputs.

MaxGekk

Can the from_csv function parse the output of to_csv() back? Could you add tests or modify existing tests, please.

HyukjinKwon

Does CSV source support this? I think it's disallowed
If we want to support this it should better be able to read it back. Problem is that CSV spec doesn't have this.

Should probably just disallow this case

LuciferYang · 2024-02-04T06:15:54Z

I think the current issue is that the original PySpark can display complex struct types,

bin/pyspark
Python 3.11.1 (v3.11.1:a7a450f84a, Dec  6 2022, 15:24:06) [Clang 13.0.0 (clang-1300.0.29.30)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
Setting default log level to "WARN".
To adjust logging level use sc.setLogLevel(newLevel). For SparkR, use setLogLevel(newLevel).
24/02/04 14:14:41 WARN NativeCodeLoader: Unable to load native-hadoop library for your platform... using builtin-java classes where applicable
24/02/04 14:14:41 WARN Utils: Service 'SparkUI' could not bind on port 4040. Attempting port 4041.
Welcome to
      ____              __
     / __/__  ___ _____/ /__
    _\ \/ _ \/ _ `/ __/  '_/
   /__ / .__/\_,_/_/ /_/\_\   version 4.0.0-SNAPSHOT
      /_/

Using Python version 3.11.1 (v3.11.1:a7a450f84a, Dec  6 2022 15:24:06)
Spark context Web UI available at http://localhost:4041
Spark context available as 'sc' (master = local[*], app id = local-1707027281550).
SparkSession available as 'spark'.
>>> from pyspark.sql import Row, functions as sf
>>> data = [(1, Row(age=2, name='Alice', scores=[100, 200, 300]))]
>>> df = spark.createDataFrame(data, ("key", "value"))
>>> df.select(sf.to_csv(df.value)).show(truncate=False)
+-----------------------+                                                       
|to_csv(value)          |
+-----------------------+
|2,Alice,"[100,200,300]"|
+-----------------------+

but PyConnect and Scala code cannot. Ultimately, we should make them consistent. Should we consider removing the current capability of PySpark(Perhaps other scenarios are not supported?)? However, this might be a breaking change.

panbingkun · 2024-02-04T08:34:08Z

Can the from_csv function parse the output of to_csv() back? Could you add tests or modify existing tests, please.

For this case, currently the from_csv function cann't parse the output of to_csv() back.
But I think after that, we can improve from_csv so that it can read back.

panbingkun · 2024-02-04T08:45:15Z

Does CSV source support this? I think it's disallowed

If we want to support this it should better be able to read it back. Problem is that CSV spec doesn't have this.

Should probably just disallow this case

I have privately discussed this issue with @LuciferYang , and in response to this situation, we actually have two options:

In to_csv When encountering complex types in CSV, such as ArrayType, MapType, StructType, we can prompt an error indicating that it is not supported.

There are two things that may seem strange:
1.In this case, in the df. show(), it can actually display complex types, which looks weird.

2.Furthermore, from the implementation of the function to_csv:

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala

Line 844 in ed6fe4f

expression[StructsToCsv]("to_csv"),

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/csvExpressions.scala

Line 245 in ed6fe4f

case class StructsToCsv(

What it means is: converting from Struct to CSV, but we don't support nested StructTypes internally, doesn't it feel a bit strange?
Another way is for us to support it. Indeed, CSV spec doesn't have this.

HyukjinKwon · 2024-02-05T05:42:59Z

I think we should simply disallow this. We could have a legacy conf, but it doesn't much make sense. What does it return for ArrayType and MapType?

…upport complex types of data

MaxGekk · 2024-03-13T12:15:15Z

+1, LGTM. Merging to master.
Thank you, @panbingkun and @HyukjinKwon @LuciferYang @srowen for review.

### What changes were proposed in this pull request? The pr aims to remove some unused error classes, include: - _LEGACY_ERROR_TEMP_3193: The code in PR(#44961) no longer uses this error class, but the related item in the file `error-classes.json` has not been deleted synchronously. - _LEGACY_ERROR_TEMP_3197: The code in PR(#44961) no longer uses this error class, but the related item in the file `error-classes.json` has not been deleted synchronously. - _LEGACY_ERROR_TEMP_3234: This should be the follow-up work of #44665 - STATE_STORE_MULTIPLE_VALUES_PER_KEY: Introduced by [SPARK-46864](#44883), but not actually used in codebase. - TWS_VALUE_SHOULD_NOT_BE_NULL: Introduced by [SPARK-46864](#44883), but not actually used in codebase. ### Why are the changes needed? Make the code cleaner. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? - Pass GA. - Manually check. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #45509 from panbingkun/remove_outdated_error_classes. Authored-by: panbingkun <panbingkun@baidu.com> Signed-off-by: Max Gekk <max.gekk@gmail.com>

cloud-fan · 2024-03-20T05:05:45Z

why do we remove a working feature? what's wrong with to_csv generating non-standard but pretty strings for these values?

MaxGekk · 2024-03-20T06:04:38Z

generating non-standard but pretty strings

@cloud-fan It prints "address" of objects, not values. Do you have an example which you worry about?

MaxGekk · 2024-03-20T06:15:36Z

@panbingkun I forgot to ask you. Does to_json have the same issue?

HyukjinKwon · 2024-03-20T07:16:05Z

to_json should be fine because it has nested type representation

cloud-fan · 2024-03-20T10:53:54Z

So we make a breaking change simply because we don't like the object address in the string? can we fix it?

panbingkun · 2024-03-20T11:37:55Z

So we make a breaking change simply because we don't like the object address in the string? can we fix it?

These types cannot be read back through 'from_csv' after generating non-standard but pretty strings through 'to_csv'.
If a type data implements toString, it can display non-standard but pretty strings, otherwise it will display the object address.
In addition, the behavior is inconsistent between Scala and Python.

Or do we need to add configuration to bring it back?

panbingkun · 2024-03-20T11:39:59Z

to_json should be fine because it has nested type representation

Yeah, there is no standard for nested types in csv format, but the json format has a standard.

cloud-fan · 2024-03-20T14:16:08Z

You never know how people will use Spark and I'm not sure why it's necessary to ban this use case. Having inconsistency between scala and python is bad. Can we fix this problem by defining a pretty string format for struct/array/map in the csv writer?

MaxGekk · 2024-03-20T15:44:31Z

You never know how people will use Spark and I'm not sure why it's necessary to ban this use case.

I believe it is not too bad to ban such output in any case (this is even insecure). @cloud-fan Do you see any use-case for that?

Array:

    val valueSchema = StructType(Seq(StructField("age", LongType), StructField("name", StringType), StructField("scores", ArrayType(LongType))))
    val schema = StructType(Seq(StructField("key", LongType), StructField("value", valueSchema)))
    val df = spark.createDataFrame(rows, schema)
    df.select(to_csv($"value")).show(false)


+--------------------------------------------------------------------------+
|to_csv(value)                                                             |
+--------------------------------------------------------------------------+
|2,Alice,org.apache.spark.sql.catalyst.expressions.UnsafeArrayData@e7bb2e54|
+--------------------------------------------------------------------------+

Map:

+------------------------------------------------------------------------+
|to_csv(value)                                                           |
+------------------------------------------------------------------------+
|2,Alice,org.apache.spark.sql.catalyst.expressions.UnsafeMapData@65ab9790|
+------------------------------------------------------------------------+

Struct:

    val rows = new java.util.ArrayList[Row]()
    rows.add(Row(1L, Row(2L, "Alice", Row(100L, 200L, null))))
    val valueSchema = StructType(Seq(StructField("age", LongType), StructField("name", StringType), StructField("scores", StructType(Seq(StructField("id1", LongType), StructField("id2", LongType), StructField("id3", LongType))))))
    val schema = StructType(Seq(StructField("key", LongType), StructField("value", valueSchema)))
    val df = spark.createDataFrame(rows, schema)

+---------------------+
|to_csv(value)        |
+---------------------+
|2,Alice,"[4,64,c8,0]"|
+---------------------+

panbingkun · 2024-03-21T00:55:27Z

Why is the result displayed through to_csv inconsistency in Scala and Python for this case?
Because this case is on the python side, it ultimately uses GenericArrayData, which happens to implement the method toString, so to_csv displays readable text.

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/GenericArrayData.scala

Line 85 in 11247d8

override def toString(): String = array.mkString("[", ",", "]")

However, on the scala side, it ultimately uses UnsafeArrayData. Unfortunately, it does not implement the method toString (using the default Object.toString method), so the final to_csv displays the address of the object.

In the implementation process of this PR, it can display non-standard but pretty strings, as follows:
9695e97

The disadvantage of this is that it cannot be read back through from_csv at present.
If the final result of the discussion is acceptable, it should be easy to bring back this feature.
Another possible compromise solution is to add a configuration (defaultly, it does not support displaying data of type [Array, Map, Struct ...] as non-standard but pretty strings through to_csv). If the user sets this configuration to be enabled, restore the original behavior?

cloud-fan · 2024-03-21T03:34:08Z

I'd prefer to fix the inconsistency. Can we use ToPrettyString.eval to generate pretty strings for these types in to_csv?

panbingkun · 2024-03-21T03:52:40Z

Use ToPrettyString.eval to generate pretty strings for these types in to_csv, I have also tried it in this PR. Below:
22a7afb

(PS: Maybe some details about using ToPrettyString.eval still need to be confirmed.)

If the final discussion result is to fix their consistency, this approach should also be possible.
WDYT @MaxGekk @HyukjinKwon @LuciferYang @xinrong-meng @srowen ?

LuciferYang · 2024-03-21T04:09:28Z

I don't oppose this resolution, but if to_csv can handle complex data structures, what would our subsequent plans be? Would from_csv also need to support complex data structures to make these paired functions more self-consistent? In the long term, do we need to enhance the read and write capabilities for CSV data sources to support complex data structures?

cloud-fan · 2024-03-21T05:19:18Z

It's ok to not support round-trip. array/struct/map is not CSV standard anyway. I think the principle is to avoid breaking changes if possible. Since to_csv already supports array/struct/map, let's keep it but fix the inconsistency issue.

cloud-fan · 2024-03-21T14:46:48Z

If no objections, I'll revert this tomorrow. We can have a new PR to add pretty string for array/struct/map in to_csv.

panbingkun · 2024-03-22T01:02:18Z

If no objections, I'll revert this tomorrow. We can have a new PR to add pretty string for array/struct/map in to_csv.

I will submit a new PR to add pretty string for array/struct/map in to_csv today.

panbingkun · 2024-03-22T03:11:42Z

@cloud-fan
The initial version has been submitted here: #45657
I am still improving it, and when it is ready, I will ask for your help in reviewing it.
Thank you.

…map/binary` as pretty strings ### What changes were proposed in this pull request? The pr aims make `to_csv` - support the output of `array/struct/map/binary` as `pretty strings`. - not support `variant`. ### Why are the changes needed? This PR was generated from follow-up comment suggestions #44665 (comment), <img width="909" alt="image" src="https://github.com/apache/spark/assets/15246973/04dd1497-da42-4b03-b21d-b041ead86f87"> ### Does this PR introduce _any_ user-facing change? Yes. ### How was this patch tested? - Update existed UT. - Pass GA. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #45657 from panbingkun/SPARK-47497. Authored-by: panbingkun <panbingkun@baidu.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

[SPARK-46654][SQL] df.show() of pyspark displayed different results b…

9fe1769

…etween Regular Spark and Spark Connect

github-actions Bot added the SQL label Jan 10, 2024

panbingkun changed the title ~~[SPARK-46654][SQL] df.show() of pyspark displayed different results between Regular Spark and Spark Connect~~ [SPARK-46654][SQL] to_csv can display complex types data Jan 11, 2024

[SPARK-46654][SQL] to_csv can display complex types data

22a7afb

panbingkun changed the title ~~[SPARK-46654][SQL] to_csv can display complex types data~~ [SPARK-46654][SQL] Make to_csv can correctly display complex types data Jan 11, 2024

panbingkun marked this pull request as ready for review January 11, 2024 02:10

panbingkun added 2 commits January 11, 2024 11:25

enable pyspark ut for to_csv

4e770cb

Merge branch 'master' into SPARK-46654

8f72454

github-actions Bot added the PYTHON label Jan 11, 2024

[SPARK-46654][SQL] Make to_csv can correctly display complex types data

bb6eec5

MaxGekk reviewed Jan 22, 2024

View reviewed changes

panbingkun added 2 commits January 24, 2024 14:52

[SPARK-46654][SQL] Make to_csv can correctly display complex types data

9695e97

[SPARK-46654][SQL] Make to_csv can correctly display complex types data

2cef851

panbingkun requested a review from MaxGekk January 24, 2024 09:14

Merge branch 'master' into SPARK-46654

ab2243b

panbingkun mentioned this pull request Jan 25, 2024

[SPARK-46850][SQL] Convert _LEGACY_ERROR_TEMP_2102 to UNSUPPORTED_DATATYPE #44871

Closed

MaxGekk reviewed Feb 3, 2024

View reviewed changes

HyukjinKwon reviewed Feb 4, 2024

View reviewed changes

panbingkun added 2 commits February 18, 2024 14:28

Merge branch 'master' into SPARK-46654

daba8aa

Merge branch 'master' into SPARK-46654

ad12661

panbingkun changed the title ~~[SPARK-46654][SQL] Make to_csv can correctly display complex types data~~ [SPARK-46654][SQL] Make to_csv explicitly indicate that it does not support complex types of data Feb 18, 2024

[SPARK-46654][SQL] Make to_csv explicitly indicate that it does not s…

5ce4eab

…upport complex types of data

github-actions Bot added the DOCS label Feb 18, 2024

fix

d012f0f

HyukjinKwon changed the title ~~[SPARK-46654][SQL][Python] Make to_csv explicitly indicate that it does not support some types of data~~ [SPARK-46654][SQL][PYTHON] Make to_csv explicitly indicate that it does not support some types of data Mar 11, 2024

MaxGekk approved these changes Mar 13, 2024

View reviewed changes

MaxGekk closed this in ccdc19d Mar 13, 2024

panbingkun mentioned this pull request Mar 14, 2024

[SPARK-47387][SQL] Remove some unused error classes #45509

Closed

panbingkun mentioned this pull request Mar 22, 2024

[SPARK-47497][SQL] Make to_csv support the output of array/struct/map/binary as pretty strings #45657

Closed

Conversation

panbingkun commented Jan 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

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

Uh oh!

panbingkun commented Jan 11, 2024

Uh oh!

LuciferYang commented Jan 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

panbingkun commented Jan 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MaxGekk Jan 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

panbingkun Jan 23, 2024

Choose a reason for hiding this comment

Uh oh!

panbingkun Jan 24, 2024

Choose a reason for hiding this comment

Uh oh!

MaxGekk left a comment

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon left a comment

Choose a reason for hiding this comment

Uh oh!

LuciferYang commented Feb 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

panbingkun commented Feb 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

panbingkun commented Feb 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HyukjinKwon commented Feb 5, 2024

Uh oh!

MaxGekk commented Mar 13, 2024

Uh oh!

cloud-fan commented Mar 20, 2024

Uh oh!

MaxGekk commented Mar 20, 2024

Uh oh!

MaxGekk commented Mar 20, 2024

Uh oh!

HyukjinKwon commented Mar 20, 2024

Uh oh!

cloud-fan commented Mar 20, 2024

Uh oh!

panbingkun commented Mar 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

panbingkun commented Mar 20, 2024

Uh oh!

cloud-fan commented Mar 20, 2024

Uh oh!

MaxGekk commented Mar 20, 2024

Uh oh!

panbingkun commented Mar 21, 2024

Uh oh!

cloud-fan commented Mar 21, 2024

Uh oh!

panbingkun commented Mar 21, 2024

Uh oh!

LuciferYang commented Mar 21, 2024

Uh oh!

cloud-fan commented Mar 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cloud-fan commented Mar 21, 2024

Uh oh!

panbingkun commented Mar 22, 2024

Uh oh!

panbingkun commented Jan 10, 2024 •

edited

Loading

LuciferYang commented Jan 11, 2024 •

edited

Loading

panbingkun commented Jan 11, 2024 •

edited

Loading

MaxGekk Jan 22, 2024 •

edited

Loading

LuciferYang commented Feb 4, 2024 •

edited

Loading

panbingkun commented Feb 4, 2024 •

edited

Loading

panbingkun commented Feb 4, 2024 •

edited

Loading

panbingkun commented Mar 20, 2024 •

edited

Loading

cloud-fan commented Mar 21, 2024 •

edited

Loading