Copilot commented on code in PR #41099:
URL: https://github.com/apache/superset/pull/41099#discussion_r3418733327
##########
superset/result_set.py:
##########
@@ -73,9 +73,16 @@ def stringify_values(array: NDArray[Any]) -> NDArray[Any]:
obj[na_obj] = None
else:
try:
- # for simple string conversions
- # this handles odd character types better
- obj[...] = obj.astype(str)
+ val = obj.item()
+ if isinstance(val, (dict, list)):
+ # Use json.dumps to produce valid double-quoted JSON.
+ # Python's str() gives single-quoted repr like {'a': 1}
+ # which is not valid JSON and breaks the frontend cell
viewer.
+ obj[...] = stringify(val)
+ else:
+ # for simple string conversions
+ # this handles odd character types better
+ obj[...] = obj.astype(str)
Review Comment:
`stringify_values` now JSON-serializes `dict`/`list` via `stringify(val)`,
which can raise `TypeError` for nested values that aren't handled by
`json_iso_dttm_ser`. This introduces a new failure mode during result set
materialization where previously `astype(str)` would always succeed. Consider
catching `TypeError` and falling back to a pessimistic JSON serializer (or
`str()`) to avoid breaking query results.
##########
tests/unit_tests/result_set_test.py:
##########
@@ -450,3 +450,71 @@ def test_stringify_extension_columns() -> None:
# plain binary BLOBs and other types are left untouched
assert pa.types.is_binary(result.schema.field("blob").type)
assert pa.types.is_integer(result.schema.field("n").type)
+
+
+def test_stringify_values_dict_and_list_produce_valid_json() -> None:
+ """
+ ClickHouse native JSON and Map types return Python dicts. When stringified
for
+ Arrow array storage they must produce valid double-quoted JSON strings, not
+ Python's single-quoted repr. Single-quoted strings pass the cheap '{'
prefix
+ check in the frontend's safeJsonObjectParse but then fail JSONbig.parse(),
+ so the SQL Lab cell viewer never activates.
+ """
+ data = np.array(
+ [
+ {"key": "value", "nested": {"a": 1}},
+ [1, 2, 3],
+ {"items": [1, 2, 3], "d": "Hello, World!"},
+ None,
+ ],
+ dtype=object,
+ )
+ result = stringify_values(data)
+
+ # Must be valid JSON strings (double-quoted), not Python repr
(single-quoted)
+ assert result[0] == '{"key": "value", "nested": {"a": 1}}'
+ assert result[1] == "[1, 2, 3]"
+ assert result[2] == '{"items": [1, 2, 3], "d": "Hello, World!"}'
+ assert result[3] is None
+
+ # Parseable by a JSON parser — confirms the frontend's JSON.parse would
succeed
+ parsed = superset_json.loads(result[0])
+ assert parsed == {"key": "value", "nested": {"a": 1}}
+ parsed = superset_json.loads(result[1])
+ assert parsed == [1, 2, 3]
Review Comment:
The list case in this test uses only integers (`[1, 2, 3]`), where Python's
repr is already valid JSON. This means the test can still pass even if lists
containing strings are incorrectly serialized with single quotes (the original
bug). Consider using a list with strings and asserting via `loads()`/prefix
checks instead of exact JSON formatting.
--
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]
