korbit-ai[bot] commented on code in PR #28268:
URL: https://github.com/apache/superset/pull/28268#discussion_r2004003720
##########
superset/result_set.py:
##########
@@ -137,6 +138,13 @@
if array.size > 0:
for column in column_names:
try:
+ # Check if array[column][0] is an list of instance of
psycopg2.extras.DateTimeTZRange
+ if isinstance(array[column][0], list) and isinstance(
+ array[column][0][0], DateTimeTZRange
+ ):
Review Comment:
### Unsafe array access in DateTimeTZRange check <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The code assumes array[column][0] and array[column][0][0] exist when
checking for DateTimeTZRange type, which could lead to IndexError if the array
is empty or contains empty lists.
###### Why this matters
If the array is empty or contains empty lists, accessing array[column][0] or
array[column][0][0] will raise an IndexError, causing the data conversion to
fail unexpectedly.
###### Suggested change ∙ *Feature Preview*
Add additional checks to safely handle empty arrays and lists:
```python
# Check if array[column] is not empty and contains
non-empty lists
if (len(array[column]) > 0 and
isinstance(array[column][0], list) and
len(array[column][0]) > 0 and
isinstance(array[column][0][0], DateTimeTZRange)):
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d3abb5bc-12a8-4b9a-af20-03df10c86db9/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d3abb5bc-12a8-4b9a-af20-03df10c86db9?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d3abb5bc-12a8-4b9a-af20-03df10c86db9?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d3abb5bc-12a8-4b9a-af20-03df10c86db9?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d3abb5bc-12a8-4b9a-af20-03df10c86db9)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:b1ed6f7e-4731-4da4-a4d9-0037f3790429 -->
[](b1ed6f7e-4731-4da4-a4d9-0037f3790429)
##########
superset/result_set.py:
##########
@@ -145,8 +153,11 @@
ValueError,
TypeError, # this is super hackey,
# https://issues.apache.org/jira/browse/ARROW-7855
- ):
+ ) as ex:
# attempt serialization of values as strings
+ logger.exception(
+ "failed to convert data to pyarrow Table %s", str(ex)
+ )
Review Comment:
### Insufficient Context in Error Log <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The log message is too generic and doesn't provide context about which
column failed during conversion.
###### Why this matters
Without column information, debugging production issues becomes more
difficult as it's unclear which specific column caused the conversion failure.
###### Suggested change ∙ *Feature Preview*
```python
logger.exception(f"failed to convert column '{column}' to pyarrow Table:
{ex}")
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8a1e8184-0bd1-4894-9826-3266ef241f8f/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8a1e8184-0bd1-4894-9826-3266ef241f8f?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8a1e8184-0bd1-4894-9826-3266ef241f8f?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8a1e8184-0bd1-4894-9826-3266ef241f8f?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8a1e8184-0bd1-4894-9826-3266ef241f8f)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:a649d6ab-3533-4a69-a562-acfba649772f -->
[](a649d6ab-3533-4a69-a562-acfba649772f)
##########
superset/result_set.py:
##########
@@ -137,6 +138,13 @@
if array.size > 0:
for column in column_names:
try:
+ # Check if array[column][0] is an list of instance of
psycopg2.extras.DateTimeTZRange
+ if isinstance(array[column][0], list) and isinstance(
+ array[column][0][0], DateTimeTZRange
+ ):
+ array[column] = [
+ [(y.lower, y.upper) for y in x] for x in
array[column]
+ ]
Review Comment:
### Missing Context for DateTimeTZRange Transformation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The newly added code handling DateTimeTZRange lacks documentation explaining
why this transformation is necessary.
###### Why this matters
Without understanding the purpose of this transformation, future maintainers
may modify or remove it incorrectly, potentially breaking PostgreSQL range type
handling.
###### Suggested change ∙ *Feature Preview*
Add comment explaining the purpose:
```python
# Convert PostgreSQL DateTimeTZRange objects into tuple pairs of (lower,
upper)
# bounds to ensure proper serialization in pyarrow
if isinstance(array[column][0], list) and isinstance(
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dea3fe98-1959-4ea8-aed7-bfcbbf1be652/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dea3fe98-1959-4ea8-aed7-bfcbbf1be652?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dea3fe98-1959-4ea8-aed7-bfcbbf1be652?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dea3fe98-1959-4ea8-aed7-bfcbbf1be652?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/dea3fe98-1959-4ea8-aed7-bfcbbf1be652)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:e9e3cdbe-bb31-4c88-8025-056727f5a1c2 -->
[](e9e3cdbe-bb31-4c88-8025-056727f5a1c2)
##########
superset/result_set.py:
##########
@@ -137,6 +138,13 @@
if array.size > 0:
for column in column_names:
try:
+ # Check if array[column][0] is an list of instance of
psycopg2.extras.DateTimeTZRange
+ if isinstance(array[column][0], list) and isinstance(
+ array[column][0][0], DateTimeTZRange
+ ):
+ array[column] = [
+ [(y.lower, y.upper) for y in x] for x in
array[column]
+ ]
Review Comment:
### Memory-intensive range conversion <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The nested list comprehension creates multiple intermediate lists and
performs tuple creation for each DateTimeTZRange element
###### Why this matters
Memory usage spikes during this operation as it creates new lists and tuples
rather than modifying data in-place, which can be problematic for large datasets
###### Suggested change ∙ *Feature Preview*
Consider using numpy's vectorized operations or preallocating the arrays to
reduce memory churn:
```python
def convert_range_array(column_data):
result = np.empty(len(column_data), dtype=object)
for i, x in enumerate(column_data):
result[i] = [(y.lower, y.upper) for y in x]
return result
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/28a89a21-25ba-43d7-aa1f-fbab567615b0/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/28a89a21-25ba-43d7-aa1f-fbab567615b0?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/28a89a21-25ba-43d7-aa1f-fbab567615b0?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/28a89a21-25ba-43d7-aa1f-fbab567615b0?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/28a89a21-25ba-43d7-aa1f-fbab567615b0)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:f18bd81b-84ea-4dd4-82d4-7d8fe9d74775 -->
[](f18bd81b-84ea-4dd4-82d4-7d8fe9d74775)
--
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]
