viirya commented on code in PR #150:
URL:
https://github.com/apache/spark-connect-rust/pull/150#discussion_r4477800188
##########
crates/pyspark-rs/src/types.rs:
##########
@@ -179,22 +179,23 @@ impl PyDataType {
}
#[classmethod]
#[pyo3(name = "fromDDL")]
- fn __obj_from_ddl(_cls: &Bound<'_, pyo3::types::PyType>, ddl: &str) ->
PyResult<PyDataType> {
- DataType::from_ddl(ddl)
- .map(PyDataType::new)
- .map_err(|e| PyErr::new::<pyo3::exceptions::PyValueError,
_>(e.to_string()))
+ fn __obj_from_ddl(cls: &Bound<'_, pyo3::types::PyType>, ddl: &str) ->
PyResult<Py<PyAny>> {
+ let dt = DataType::from_ddl(ddl)
+ .map_err(|e| PyErr::new::<pyo3::exceptions::PyValueError,
_>(e.to_string()))?;
+ // The concrete type, as pyspark returns (a StructType for a list of
fields).
+ data_type_to_py(cls.py(), &dt)
Review Comment:
With this change, `PyDataType::new` has no callers left, so it can be
removed. Also, the comment in `py_to_data_type` saying the bare base `DataType`
comes "e.g. from `fromDDL`" is now stale. Only `DataType()` and pure-Python
subclasses reach that branch.
##########
crates/pyspark-rs/src/types.rs:
##########
@@ -235,10 +236,11 @@ impl PyNullType {
}
#[classmethod]
#[pyo3(name = "fromDDL")]
- fn __obj_from_ddl(_cls: &Bound<'_, pyo3::types::PyType>, ddl: &str) ->
PyResult<PyDataType> {
- DataType::from_ddl(ddl)
- .map(PyDataType::new)
- .map_err(|e| PyErr::new::<pyo3::exceptions::PyValueError,
_>(e.to_string()))
+ fn __obj_from_ddl(cls: &Bound<'_, pyo3::types::PyType>, ddl: &str) ->
PyResult<Py<PyAny>> {
Review Comment:
These per-subclass `fromDDL` copies can be dropped. The classmethod on
`DataType` is inherited: `GeometryType` has no copy of its own, and
`GeometryType.fromDDL("int")` already returns `IntegerType()`. Keeping only the
base one removes all 24 copies, along with their repeated comment.
##########
python/tests/test_dropin_offline.py:
##########
@@ -419,13 +419,62 @@ def test_structtype_add_reflected_in_json():
def test_structtype_add_reflected_in_the_base_data_type():
- # Regression: add() must update the base DataType too, which str() and the
schemas of
- # mapInArrow/mapInPandas/applyInPandas/... read (via resolve_datatype).
+ # Regression: add() must update the base DataType too, which the schemas of
+ # mapInArrow/mapInPandas/applyInPandas/... read (via resolve_datatype).
The base
+ # implementation of simpleString reads it.
built = T.StructType([T.StructField("a", T.LongType()), T.StructField("b",
T.LongType())])
- assert str(T.StructType().add("a", T.LongType()).add("b", T.LongType()))
== str(built)
+ chained = T.StructType().add("a", T.LongType()).add("b", T.LongType())
st = T.StructType([T.StructField("a", T.LongType())])
st.add("b", T.LongType())
- assert str(st) == str(built) == "struct<a:bigint,b:bigint>"
+ for added in (chained, st):
+ assert T.DataType.simpleString(added) == "struct<a:bigint,b:bigint>"
+ assert str(added) == str(built) == repr(built)
+
+
[email protected]("data_type,expected", [
+ (T.LongType(), "LongType()"),
+ (T.TimestampNTZType(), "TimestampNTZType()"),
+ (T.VariantType(), "VariantType()"),
+ (T.DecimalType(10, 2), "DecimalType(10,2)"),
+ (T.CharType(5), "CharType(5)"),
+ (T.DayTimeIntervalType(0, 3), "DayTimeIntervalType(0, 3)"),
+ (T.YearMonthIntervalType(), "YearMonthIntervalType(0, 1)"),
+ (T.ArrayType(T.StringType(), False), "ArrayType(StringType(), False)"),
+ (T.MapType(T.StringType(), T.LongType()), "MapType(StringType(),
LongType(), True)"),
+ (T.StructType([T.StructField("a", T.LongType()),
+ T.StructField("b", T.ArrayType(T.StringType()), False)]),
+ "StructType([StructField('a', LongType(), True), "
+ "StructField('b', ArrayType(StringType(), True), False)])"),
+ (T.StructType(), "StructType([])"),
+ (T.DataType(), "DataType()"),
+])
+def test_datatype_str_is_its_repr_as_pyspark(data_type, expected):
+ # pyspark data types define only __repr__, so str() is the same.
+ assert repr(data_type) == expected
+ assert str(data_type) == expected
+
+
+def test_user_defined_type_repr_is_its_class_name():
+ from pyspark.testing.objects import ExamplePointUDT
+
+ assert repr(ExamplePointUDT()) == str(ExamplePointUDT()) ==
"ExamplePointUDT()"
+
+
[email protected]("ddl,expected", [
+ # The examples of pyspark's DataType.fromDDL.
+ ("b string, a int",
+ "StructType([StructField('b', StringType(), True), StructField('a',
IntegerType(), True)])"),
+ ("decimal(10,10)", "DecimalType(10,10)"),
+ ("b: string, a: int",
+ "StructType([StructField('b', StringType(), True), StructField('a',
IntegerType(), True)])"),
+ ("int", "IntegerType()"),
+])
+def test_datatype_from_ddl_returns_the_concrete_type(ddl, expected):
+ parsed = T.DataType.fromDDL(ddl)
+ assert repr(parsed) == expected
+ assert type(parsed).__name__ == expected.split("(")[0]
Review Comment:
nit: the repr assertion above mostly covers this already. If you want an
explicit type check, `isinstance(parsed, T.StructType)` and so on reads more
clearly than comparing `__name__`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
