You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/12/02 06:46:15 UTC
[GitHub] [spark] LuciferYang opened a new pull request, #38873: [SPARK-41358][SQL] Refactor `ColumnVectorUtils` to use `PhysicalDataType` instead of `DataType`
LuciferYang opened a new pull request, #38873:
URL: https://github.com/apache/spark/pull/38873
### What changes were proposed in this pull request?
The main change of this pr as follows:
- Introduce `PhysicalDataTypes` similar to `DataTypes` to et specific physical data type object.
- Refactor `ColumnVectorUtils` to use `PhysicalDataType` instead of `DataType`.
### Why are the changes needed?
Simplify type match.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Pass GitHub Actions
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #38873: [SPARK-41358][SQL] Refactor `ColumnVectorUtils` to use `PhysicalDataType` instead of `DataType`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #38873:
URL: https://github.com/apache/spark/pull/38873#discussion_r1037863025
##########
sql/catalyst/src/main/java/org/apache/spark/sql/types/PhysicalDataTypes.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.types;
+
+import org.apache.spark.sql.catalyst.types.*;
+
+/**
+ * To get specific physical data type.
+ *
+ * @since 3.4.0
+ */
+public class PhysicalDataTypes {
Review Comment:
done
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #38873: [SPARK-41358][SQL] Refactor `ColumnVectorUtils` to use `PhysicalDataType` instead of `DataType`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #38873:
URL: https://github.com/apache/spark/pull/38873#discussion_r1037847096
##########
sql/catalyst/src/main/java/org/apache/spark/sql/types/PhysicalDataTypes.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.types;
+
+import org.apache.spark.sql.catalyst.types.*;
+
+/**
+ * To get specific physical data type.
+ *
+ * @since 3.4.0
+ */
+public class PhysicalDataTypes {
Review Comment:
If it's not ugly, I think `PhysicalDataTypes` can be deleted
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #38873: [SPARK-41358][SQL] Refactor `ColumnVectorUtils#populate` method to use `PhysicalDataType` instead of `DataType`
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #38873:
URL: https://github.com/apache/spark/pull/38873#discussion_r1037917365
##########
sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/ColumnVectorUtils.java:
##########
@@ -125,32 +125,45 @@ public static Map<Integer, Integer> toJavaIntMap(ColumnarMap map) {
}
private static void appendValue(WritableColumnVector dst, DataType t, Object o) {
Review Comment:
Yea, this is an existing bug and should be fixed separately.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #38873: [SPARK-41358][SQL] Refactor `ColumnVectorUtils` to use `PhysicalDataType` instead of `DataType`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #38873:
URL: https://github.com/apache/spark/pull/38873#discussion_r1037856173
##########
sql/catalyst/src/main/java/org/apache/spark/sql/types/PhysicalDataTypes.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.types;
+
+import org.apache.spark.sql.catalyst.types.*;
+
+/**
+ * To get specific physical data type.
+ *
+ * @since 3.4.0
+ */
+public class PhysicalDataTypes {
Review Comment:
OK, let me remove PhysicalDataTypes and change to use `isinstanceof `
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #38873: [SPARK-41358][SQL] Refactor `ColumnVectorUtils#populate` method to use `PhysicalDataType` instead of `DataType`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #38873:
URL: https://github.com/apache/spark/pull/38873#discussion_r1039140895
##########
sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/ColumnVectorUtils.java:
##########
@@ -125,32 +125,45 @@ public static Map<Integer, Integer> toJavaIntMap(ColumnarMap map) {
}
private static void appendValue(WritableColumnVector dst, DataType t, Object o) {
Review Comment:
> Yea, this is an existing bug and should be fixed separately.
thanks @cloud-fan, I will follow up on the remaining bug fix later
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #38873: [SPARK-41358][SQL] Refactor `ColumnVectorUtils#populate` method to use `PhysicalDataType` instead of `DataType`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #38873:
URL: https://github.com/apache/spark/pull/38873#discussion_r1039618680
##########
sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/ColumnVectorUtils.java:
##########
@@ -125,32 +125,45 @@ public static Map<Integer, Integer> toJavaIntMap(ColumnarMap map) {
}
private static void appendValue(WritableColumnVector dst, DataType t, Object o) {
Review Comment:
`ColumnVectorUtils#toBatch` and related `appendValue` methods will only be used by test cases
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #38873: [SPARK-41358][SQL] Refactor `ColumnVectorUtils` to use `PhysicalDataType` instead of `DataType`
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #38873:
URL: https://github.com/apache/spark/pull/38873#discussion_r1037874336
##########
sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/ColumnVectorUtils.java:
##########
@@ -125,32 +122,37 @@ public static Map<Integer, Integer> toJavaIntMap(ColumnarMap map) {
}
private static void appendValue(WritableColumnVector dst, DataType t, Object o) {
+ PhysicalDataType pdt = t.physicalDataType();
if (o == null) {
- if (t instanceof CalendarIntervalType) {
+ if (pdt instanceof PhysicalCalendarIntervalType) {
dst.appendStruct(true);
} else {
dst.appendNull();
}
} else {
- if (t == DataTypes.BooleanType) {
+ if (pdt instanceof PhysicalBooleanType) {
dst.appendBoolean((Boolean) o);
- } else if (t == DataTypes.ByteType) {
+ } else if (pdt instanceof PhysicalByteType) {
dst.appendByte((Byte) o);
- } else if (t == DataTypes.ShortType) {
+ } else if (pdt instanceof PhysicalShortType) {
dst.appendShort((Short) o);
- } else if (t == DataTypes.IntegerType) {
- dst.appendInt((Integer) o);
- } else if (t == DataTypes.LongType) {
+ } else if (pdt instanceof PhysicalIntegerType) {
+ if (o instanceof Date) {
+ dst.appendInt(DateTimeUtils.fromJavaDate((Date) o));
Review Comment:
This is a bit weird. Why the caller side passes `Date` for date type but `Long` for timestamp type?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #38873: [SPARK-41358][SQL] Refactor `ColumnVectorUtils` to use `PhysicalDataType` instead of `DataType`
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #38873:
URL: https://github.com/apache/spark/pull/38873#discussion_r1037849024
##########
sql/catalyst/src/main/java/org/apache/spark/sql/types/PhysicalDataTypes.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.types;
+
+import org.apache.spark.sql.catalyst.types.*;
+
+/**
+ * To get specific physical data type.
+ *
+ * @since 3.4.0
+ */
+public class PhysicalDataTypes {
Review Comment:
It's OK. This is internal code anyway.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on pull request #38873: [SPARK-41358][SQL] Refactor `ColumnVectorUtils` to use `PhysicalDataType` instead of `DataType`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #38873:
URL: https://github.com/apache/spark/pull/38873#issuecomment-1334830164
@cloud-fan @desmondcheongzx I create an Umbrella SPARK-41356 and add 2 possible initial tasks and will add subTasks gradually in the future, If you have time, please help to review the first one. Thanks ~
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #38873: [SPARK-41358][SQL] Refactor `ColumnVectorUtils` to use `PhysicalDataType` instead of `DataType`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #38873:
URL: https://github.com/apache/spark/pull/38873#discussion_r1037846604
##########
sql/catalyst/src/main/java/org/apache/spark/sql/types/PhysicalDataTypes.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.types;
+
+import org.apache.spark.sql.catalyst.types.*;
+
+/**
+ * To get specific physical data type.
+ *
+ * @since 3.4.0
+ */
+public class PhysicalDataTypes {
Review Comment:
For example, in the following scenario:
https://github.com/apache/spark/blob/3fc8a90267312fdfc42b7e6a16ee69f2507ef56b/sql/catalyst/src/main/java/org/apache/spark/sql/catalyst/expressions/UnsafeRow.java#L79-L95
if we can replace `PhysicalDataType` with `DataType`:
```java
mutableFieldTypes = Collections.unmodifiableSet(
new HashSet<>(
Arrays.asList(
PhysicalNullType$.MODULE$,
PhysicalBooleanType$.MODULE$,
PhysicalByteType$.MODULE$,
PhysicalShortType$.MODULE$,
PhysicalIntegerType$.MODULE$,
PhysicalLongType$.MODULE$,
PhysicalFloatType$.MODULE$,
PhysicalDoubleType$.MODULE$
)));
```
Does it look ugly
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #38873: [SPARK-41358][SQL] Refactor `ColumnVectorUtils` to use `PhysicalDataType` instead of `DataType`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #38873:
URL: https://github.com/apache/spark/pull/38873#discussion_r1037892873
##########
sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/ColumnVectorUtils.java:
##########
@@ -125,32 +122,37 @@ public static Map<Integer, Integer> toJavaIntMap(ColumnarMap map) {
}
private static void appendValue(WritableColumnVector dst, DataType t, Object o) {
+ PhysicalDataType pdt = t.physicalDataType();
if (o == null) {
- if (t instanceof CalendarIntervalType) {
+ if (pdt instanceof PhysicalCalendarIntervalType) {
dst.appendStruct(true);
} else {
dst.appendNull();
}
} else {
- if (t == DataTypes.BooleanType) {
+ if (pdt instanceof PhysicalBooleanType) {
dst.appendBoolean((Boolean) o);
- } else if (t == DataTypes.ByteType) {
+ } else if (pdt instanceof PhysicalByteType) {
dst.appendByte((Byte) o);
- } else if (t == DataTypes.ShortType) {
+ } else if (pdt instanceof PhysicalShortType) {
dst.appendShort((Short) o);
- } else if (t == DataTypes.IntegerType) {
- dst.appendInt((Integer) o);
- } else if (t == DataTypes.LongType) {
+ } else if (pdt instanceof PhysicalIntegerType) {
+ if (o instanceof Date) {
+ dst.appendInt(DateTimeUtils.fromJavaDate((Date) o));
Review Comment:
https://github.com/apache/spark/blob/5d90f9825bc5aad1cf24b773586cba5309ee6caa/sql/catalyst/src/main/scala/org/apache/spark/sql/Row.scala#L170-L174
Except for `java.sql.Timestamp`, I think we should also handle `o` is `java.time.LocalDate` and `java.time.Instant` ref the document.?
There should be no tests to cover these branches. I will add some test later.
##########
sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/ColumnVectorUtils.java:
##########
@@ -125,32 +122,37 @@ public static Map<Integer, Integer> toJavaIntMap(ColumnarMap map) {
}
private static void appendValue(WritableColumnVector dst, DataType t, Object o) {
+ PhysicalDataType pdt = t.physicalDataType();
if (o == null) {
- if (t instanceof CalendarIntervalType) {
+ if (pdt instanceof PhysicalCalendarIntervalType) {
dst.appendStruct(true);
} else {
dst.appendNull();
}
} else {
- if (t == DataTypes.BooleanType) {
+ if (pdt instanceof PhysicalBooleanType) {
dst.appendBoolean((Boolean) o);
- } else if (t == DataTypes.ByteType) {
+ } else if (pdt instanceof PhysicalByteType) {
dst.appendByte((Byte) o);
- } else if (t == DataTypes.ShortType) {
+ } else if (pdt instanceof PhysicalShortType) {
dst.appendShort((Short) o);
- } else if (t == DataTypes.IntegerType) {
- dst.appendInt((Integer) o);
- } else if (t == DataTypes.LongType) {
+ } else if (pdt instanceof PhysicalIntegerType) {
+ if (o instanceof Date) {
+ dst.appendInt(DateTimeUtils.fromJavaDate((Date) o));
Review Comment:
https://github.com/apache/spark/blob/5d90f9825bc5aad1cf24b773586cba5309ee6caa/sql/catalyst/src/main/scala/org/apache/spark/sql/Row.scala#L170-L174
Except for `java.sql.Timestamp`, I think we should also handle `o` is `java.time.LocalDate` and `java.time.Instant` ref to the document.?
There should be no tests to cover these branches. I will add some test later.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #38873: [SPARK-41358][SQL] Refactor `ColumnVectorUtils` to use `PhysicalDataType` instead of `DataType`
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #38873:
URL: https://github.com/apache/spark/pull/38873#discussion_r1037840537
##########
sql/catalyst/src/main/java/org/apache/spark/sql/types/PhysicalDataTypes.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.types;
+
+import org.apache.spark.sql.catalyst.types.*;
+
+/**
+ * To get specific physical data type.
+ *
+ * @since 3.4.0
+ */
+public class PhysicalDataTypes {
Review Comment:
is this really needed? `PhysicalDataType` is an internal API, and we don't need to make it java friendly. We can write `df isinstanceof PhysicalStringType`.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #38873: [SPARK-41358][SQL] Refactor `ColumnVectorUtils` to use `PhysicalDataType` instead of `DataType`
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on code in PR #38873:
URL: https://github.com/apache/spark/pull/38873#discussion_r1037897941
##########
sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/ColumnVectorUtils.java:
##########
@@ -125,32 +125,45 @@ public static Map<Integer, Integer> toJavaIntMap(ColumnarMap map) {
}
private static void appendValue(WritableColumnVector dst, DataType t, Object o) {
Review Comment:
If this function is to handle external types (Row), I think matching logical data type makes more sense, as external types are bound to logical types.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan closed pull request #38873: [SPARK-41358][SQL] Refactor `ColumnVectorUtils#populate` method to use `PhysicalDataType` instead of `DataType`
Posted by GitBox <gi...@apache.org>.
cloud-fan closed pull request #38873: [SPARK-41358][SQL] Refactor `ColumnVectorUtils#populate` method to use `PhysicalDataType` instead of `DataType`
URL: https://github.com/apache/spark/pull/38873
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on pull request #38873: [SPARK-41358][SQL] Refactor `ColumnVectorUtils#populate` method to use `PhysicalDataType` instead of `DataType`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #38873:
URL: https://github.com/apache/spark/pull/38873#issuecomment-1336003915
GA passed
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on pull request #38873: [SPARK-41358][SQL] Refactor `ColumnVectorUtils#populate` method to use `PhysicalDataType` instead of `DataType`
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on PR #38873:
URL: https://github.com/apache/spark/pull/38873#issuecomment-1336749026
thanks, merging to master!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #38873: [SPARK-41358][SQL] Refactor `ColumnVectorUtils` to use `PhysicalDataType` instead of `DataType`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #38873:
URL: https://github.com/apache/spark/pull/38873#discussion_r1037892873
##########
sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/ColumnVectorUtils.java:
##########
@@ -125,32 +122,37 @@ public static Map<Integer, Integer> toJavaIntMap(ColumnarMap map) {
}
private static void appendValue(WritableColumnVector dst, DataType t, Object o) {
+ PhysicalDataType pdt = t.physicalDataType();
if (o == null) {
- if (t instanceof CalendarIntervalType) {
+ if (pdt instanceof PhysicalCalendarIntervalType) {
dst.appendStruct(true);
} else {
dst.appendNull();
}
} else {
- if (t == DataTypes.BooleanType) {
+ if (pdt instanceof PhysicalBooleanType) {
dst.appendBoolean((Boolean) o);
- } else if (t == DataTypes.ByteType) {
+ } else if (pdt instanceof PhysicalByteType) {
dst.appendByte((Byte) o);
- } else if (t == DataTypes.ShortType) {
+ } else if (pdt instanceof PhysicalShortType) {
dst.appendShort((Short) o);
- } else if (t == DataTypes.IntegerType) {
- dst.appendInt((Integer) o);
- } else if (t == DataTypes.LongType) {
+ } else if (pdt instanceof PhysicalIntegerType) {
+ if (o instanceof Date) {
+ dst.appendInt(DateTimeUtils.fromJavaDate((Date) o));
Review Comment:
https://github.com/apache/spark/blob/5d90f9825bc5aad1cf24b773586cba5309ee6caa/sql/catalyst/src/main/scala/org/apache/spark/sql/Row.scala#L170-L174
Except for `java.sql.Timestamp ', I think we should also handle `o` is `java.time.LocalDate` and `java.time.Instant` ref the document.?
There should be no tests to cover these branches. I will add some test later.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #38873: [SPARK-41358][SQL] Refactor `ColumnVectorUtils` to use `PhysicalDataType` instead of `DataType`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #38873:
URL: https://github.com/apache/spark/pull/38873#discussion_r1037904653
##########
sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/ColumnVectorUtils.java:
##########
@@ -125,32 +125,45 @@ public static Map<Integer, Integer> toJavaIntMap(ColumnarMap map) {
}
private static void appendValue(WritableColumnVector dst, DataType t, Object o) {
Review Comment:
OK, I will revert the change of this method, but there maybe 2 problem:
- TimestampType is not handled, will throw `UnsupportedOperationException` directly
- appendValue method not handle MapType yet
seems that another pr is needed to solve these?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org