[SPARK-39168][PYTHON] Use all values in a python list when inferring ArrayType schema #36545

physinet · 2022-05-13T20:28:52Z

What changes were proposed in this pull request?

This PR modifies type inference for python lists to consider all values in the list, not just the first value.

Why are the changes needed?

This enables convenient type inference in the two following cases:

	previous	current
`[None, 1]`	`array<void>` (raises `ValueError`)	`array<bigint>`
`[{"b": 1}, {"c": 2}]`	`array<struct<b:bigint>>`	`array<struct<b:bigint,c:bigint>>`

Does this PR introduce any user-facing change?

Possible user-facing changes:

attempting to infer the schema of an array with mixed types (e.g. ["a", 1]) may result in a TypeError
- previously, this was inferred as an array<string> and produced a value ["a", "1"]
fields of inferred struct types will differ if dictionaries in an array have different keys

How was this patch tested?

Added unit tests for various cases.

physinet · 2022-05-13T20:31:33Z

Add a unit test for ["a", 1] producing a TypeError
Consider making this behavior configurable

python/pyspark/sql/tests/test_types.py

HyukjinKwon · 2022-05-14T06:52:27Z

Nice PR description. Yeah, we should probably add a configuration then, please also refer to 2537fe8 for adding a SQL config

HyukjinKwon · 2022-05-14T06:56:55Z

We should probably add a configuration like spark.sql.pyspark.legacy.inferFirstElementInArray.enabled (feel free to pick other names if you have other ideas). Would also have to create a new page like https://github.com/apache/spark/blob/master/python/docs/source/migration_guide/pyspark_3.2_to_3.3.rst and describe this behavior change.

AmplabJenkins · 2022-05-15T00:18:04Z

Can one of the admins verify this patch?

physinet · 2022-05-17T01:19:21Z

We should probably add a configuration like spark.sql.pyspark.legacy.inferFirstElementInArray.enabled (feel free to pick other names if you have other ideas). Would also have to create a new page like https://github.com/apache/spark/blob/master/python/docs/source/migration_guide/pyspark_3.2_to_3.3.rst and describe this behavior change.

Added this configuration (slightly changed the name) and created a new migration guide page.

python/docs/source/migration_guide/pyspark_3.3_to_3.4.rst

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

HyukjinKwon

LGTM otherwise

HyukjinKwon · 2022-05-17T03:50:04Z

cc @BryanCutler @viirya @ueshin FYI

srowen · 2022-05-17T13:46:50Z

python/pyspark/sql/session.py

@@ -570,10 +570,20 @@ def _inferSchemaFromList(
        if not data:
            raise ValueError("can not infer schema from empty dataset")
        infer_dict_as_struct = self._jconf.inferDictAsStruct()
+        infer_array_from_first_element = self._jconf.legacyInferArrayTypeFromFirstElement()


Do we really need a flag? in the cases where this matters, the schema would be wrong and produce an error; would that be desirable?

Previously it was allowed to have mixed types in a python list, as long as the types could be cast to the type enforced by the schema inferred from the first element:

>>> df = spark.createDataFrame([{"a": ["1", 2]}]) >>> df.show() +------+ | a| +------+ |[1, 2]| +------+ >>> df.schema StructType(List(StructField(a,ArrayType(StringType,true),true)))

With this change, creating the DataFrame causes an error:

>>> df = spark.createDataFrame([{"a": ["1", 2]}]) ... TypeError: Unable to infer the type of the field a.

Because of this change, I think it makes sense to have the behavior configurable.

Wait, this seems like the wrong behavior. StringType should be inferred for this column, as it is now. I don't see a reason to break this? don't we want to just find the widest applicable type?

I can see the argument for casting to the widest applicable type, but I think that should be a separate discussion. Inferring the type of an array I think should be analogous to inferring a type over rows, which raises an error in this case:

>>> spark.createDataFrame([{"a": "1"}, {"a": 2}]) ... TypeError: field a: Can not merge type <class 'pyspark.sql.types.StringType'> and <class 'pyspark.sql.types.LongType'>

I see it fixes some cases, by somewhat 'accidentally' correctly inferring a widening type. But it does cause some common cases to start failing, when they 'accidentally' work now (your example in this thread). It feels like the half-measure isn't worth it, as it needs a whole new flag. Is it hard to just implement logic to find the closest type for everything? that code surely already exists in the code base

This seems to be marked as a TODO (at least on the python side). https://github.com/apache/spark/blob/master/python/pyspark/sql/types.py#L1377

Again, I think this should be addressed separately from this PR, but I'm happy to hold off on this PR if you think type-widening logic should be implemented first.

I am not super familiar with this part of the code and I don't object to the change if someone else wants to merge; just seems like it would be relatively straightforward to 'really' fix it

Yeah, that's right that it causes a behaviour change. However, the (previous) string type coercion behaviour in an element of an array is actually a mistake I believe. For example, such type coercion is not supported in regular type inference:

>>> spark.createDataFrame([{"a": "1"}, {"a" :2}]) Traceback (most recent call last): ... TypeError: field a: Can not merge type <class 'pyspark.sql.types.StringType'> and <class 'pyspark.sql.types.LongType'>

So, what this PR actually does is to match the behaviour with non-nested type inference. The switch was added for users dependent on the previous behaviour.

I see, that is sounding reasonable then to me. I dislike flags, but, if a follow-up made the flag irrelevant, maybe that's OK.

By the way, I think StringType is the only type that supports coercion currently: https://github.com/apache/spark/blob/master/python/pyspark/sql/types.py#L1596-L1599

viirya

Having a quick look. The new behavior seems more reasonable, although there is a concern on breaking current behavior. Migration guide is updated and a legacy flag is added. So seems okay to go.

HyukjinKwon · 2022-05-19T01:06:06Z

Merged to master.

HyukjinKwon · 2022-05-19T01:07:30Z

Thanks for working on this @physinet, and congrats for being a Apache Spark contributor :-).

…rArrayTypeFromFirstElement ### What changes were proposed in this pull request? This PR fixes a bug that does not respect `spark.sql.pyspark.legacy.inferArrayTypeFromFirstElement.enabled` in nested arrays, introduced by #36545. ### Why are the changes needed? To have a way to restore the original behaviour. ### Does this PR introduce _any_ user-facing change? Yes, it fixes the regression when `spark.sql.pyspark.legacy.inferArrayTypeFromFirstElement.enabled` is set to `True`. ### How was this patch tested? Unittest added. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #46548 from HyukjinKwon/SPARK-48248. Authored-by: Hyukjin Kwon <gurwls223@apache.org> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>

…rArrayTypeFromFirstElement This PR fixes a bug that does not respect `spark.sql.pyspark.legacy.inferArrayTypeFromFirstElement.enabled` in nested arrays, introduced by #36545. To have a way to restore the original behaviour. Yes, it fixes the regression when `spark.sql.pyspark.legacy.inferArrayTypeFromFirstElement.enabled` is set to `True`. Unittest added. No. Closes #46548 from HyukjinKwon/SPARK-48248. Authored-by: Hyukjin Kwon <gurwls223@apache.org> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org> (cherry picked from commit b2140d0) Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>

… schema ### What changes were proposed in this pull request? This is similar with #36545. This PR proposes to infer the map types from all pairs instead of the first pair. ### Why are the changes needed? To have the consistent behaivor. e.g., ```python >>> spark.createDataFrame([[1], [2], ["a"], ["c"]]).collect() [Row(_1='1'), Row(_1='2'), Row(_1='a'), Row(_1='c')] ``` ### Does this PR introduce _any_ user-facing change? Yes. See below **Without Spark Connect:** ```python >>> spark.createDataFrame([{"outer": {"payment": 200.5, "name": "A"}}]).collect() [Row(outer={'name': 'A', 'payment': '200.5'})] >>> spark.conf.set("spark.sql.pyspark.legacy.inferMapTypeFromFirstPair.enabled", True) >>> spark.createDataFrame([{"outer": {"payment": 200.5, "name": "A"}}]).collect() [Row(outer={'name': None, 'payment': 200.5})] ``` **With Spark Conenct:** ```python >>> spark.createDataFrame([{"outer": {"payment": 200.5, "name": "A"}}]).collect() [Row(outer={'payment': '200.5', 'name': 'A'})] >>> spark.conf.set("spark.sql.pyspark.legacy.inferMapTypeFromFirstPair.enabled", True) >>> spark.createDataFrame([{"outer": {"payment": 200.5, "name": "A"}}]).collect() Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/.../spark/python/pyspark/sql/connect/session.py", line 635, in createDataFrame _table = LocalDataToArrowConversion.convert(_data, _schema) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/.../spark/python/pyspark/sql/connect/conversion.py", line 378, in convert return pa.Table.from_arrays(pylist, schema=pa_schema) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "pyarrow/table.pxi", line 3974, in pyarrow.lib.Table.from_arrays File "pyarrow/table.pxi", line 1464, in pyarrow.lib._sanitize_arrays File "pyarrow/array.pxi", line 373, in pyarrow.lib.asarray File "pyarrow/array.pxi", line 343, in pyarrow.lib.array File "pyarrow/array.pxi", line 42, in pyarrow.lib._sequence_to_array File "pyarrow/error.pxi", line 154, in pyarrow.lib.pyarrow_internal_check_status File "pyarrow/error.pxi", line 91, in pyarrow.lib.check_status pyarrow.lib.ArrowInvalid: Could not convert 'A' with type str: tried to convert to double ``` ### How was this patch tested? Unittests added ### Was this patch authored or co-authored using generative AI tooling? No. Closes #46547 from HyukjinKwon/infer-map-first. Lead-authored-by: Hyukjin Kwon <gurwls223@apache.org> Co-authored-by: Hyukjin Kwon <gurwls223@gmail.com> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>

…rArrayTypeFromFirstElement This PR fixes a bug that does not respect `spark.sql.pyspark.legacy.inferArrayTypeFromFirstElement.enabled` in nested arrays, introduced by apache#36545. To have a way to restore the original behaviour. Yes, it fixes the regression when `spark.sql.pyspark.legacy.inferArrayTypeFromFirstElement.enabled` is set to `True`. Unittest added. No. Closes apache#46548 from HyukjinKwon/SPARK-48248. Authored-by: Hyukjin Kwon <gurwls223@apache.org> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org> (cherry picked from commit b2140d0) Signed-off-by: Hyukjin Kwon <gurwls223@apache.org> (cherry picked from commit 1d6724ca8e9e79f666aefd4258cce1482602644f)

physinet added 3 commits May 12, 2022 16:06

infer schema considering all array values

254c4a7

support empty arrays

7c31c8a

array type merging linting

d0f0b95

github-actions bot added CORE PYTHON SQL labels May 13, 2022

add test for conflicting types in array

bee22e5

physinet changed the title ~~[WIP][SPARK-39168] Use all values in a python list when inferring ArrayType schema~~ [WIP][SPARK-39168][PYTHON] Use all values in a python list when inferring ArrayType schema May 13, 2022

HyukjinKwon reviewed May 14, 2022

View reviewed changes

python/pyspark/sql/tests/test_types.py Show resolved Hide resolved

physinet added 5 commits May 16, 2022 10:56

Merge branch 'apache:master' into infer_schema_from_full_array

4944cb4

add configuration parameter

e1b0e44

fix linting

9abdb30

add migration guide to toctree

ab80a15

fix tests

a6b207e

HyukjinKwon reviewed May 17, 2022

View reviewed changes

python/docs/source/migration_guide/pyspark_3.3_to_3.4.rst Outdated Show resolved Hide resolved

HyukjinKwon reviewed May 17, 2022

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala Outdated Show resolved Hide resolved

HyukjinKwon approved these changes May 17, 2022

View reviewed changes

PR feedback

7cde786

physinet force-pushed the infer_schema_from_full_array branch from cfc81be to 7cde786 Compare May 17, 2022 12:58

physinet changed the title ~~[WIP][SPARK-39168][PYTHON] Use all values in a python list when inferring ArrayType schema~~ [SPARK-39168][PYTHON] Use all values in a python list when inferring ArrayType schema May 17, 2022

srowen reviewed May 17, 2022

View reviewed changes

viirya reviewed May 18, 2022

View reviewed changes

HyukjinKwon closed this in 17a39d0 May 19, 2022

This was referenced May 13, 2024

[SPARK-48247][PYTHON] Use all values in a dict when inferring MapType schema #46547

Closed

[SPARK-48248][PYTHON] Fix nested array to respect legacy conf of inferArrayTypeFromFirstElement #46548

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-39168][PYTHON] Use all values in a python list when inferring ArrayType schema #36545

[SPARK-39168][PYTHON] Use all values in a python list when inferring ArrayType schema #36545

physinet commented May 13, 2022

physinet commented May 13, 2022 •

edited

Loading

HyukjinKwon commented May 14, 2022

HyukjinKwon commented May 14, 2022 •

edited

Loading

AmplabJenkins commented May 15, 2022

physinet commented May 17, 2022

HyukjinKwon left a comment

HyukjinKwon commented May 17, 2022

srowen May 17, 2022

physinet May 17, 2022

srowen May 17, 2022

physinet May 17, 2022

srowen May 17, 2022

physinet May 17, 2022

srowen May 17, 2022

HyukjinKwon May 17, 2022

srowen May 17, 2022

physinet May 18, 2022 •

edited

Loading

viirya left a comment •

edited

Loading

HyukjinKwon commented May 19, 2022

HyukjinKwon commented May 19, 2022

[SPARK-39168][PYTHON] Use all values in a python list when inferring ArrayType schema #36545

[SPARK-39168][PYTHON] Use all values in a python list when inferring ArrayType schema #36545

Conversation

physinet commented May 13, 2022

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?

physinet commented May 13, 2022 • edited Loading

HyukjinKwon commented May 14, 2022

HyukjinKwon commented May 14, 2022 • edited Loading

AmplabJenkins commented May 15, 2022

physinet commented May 17, 2022

HyukjinKwon left a comment

Choose a reason for hiding this comment

HyukjinKwon commented May 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

physinet May 18, 2022 • edited Loading

Choose a reason for hiding this comment

viirya left a comment • edited Loading

Choose a reason for hiding this comment

HyukjinKwon commented May 19, 2022

HyukjinKwon commented May 19, 2022

physinet commented May 13, 2022 •

edited

Loading

HyukjinKwon commented May 14, 2022 •

edited

Loading

physinet May 18, 2022 •

edited

Loading

viirya left a comment •

edited

Loading