You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "madhushreeag (via GitHub)" <gi...@apache.org> on 2026/06/02 06:29:14 UTC
[PR] fix(dataset):add feature flag for preserving numeric columns for special floats [superset]
madhushreeag opened a new pull request, #40677:
URL: https://github.com/apache/superset/pull/40677
### SUMMARY
pydruid infers column types from the first row value of each column in a result set. This creates two related problems for numeric columns:
1. IEEE special floats: Druid cannot represent NaN, Infinity, or -Infinity in JSON, so pydruid emits them as the strings "NaN", "Infinity", and "-Infinity". When one of these strings appears in an otherwise numeric column, pa.array() raises ArrowInvalid on the mixed str/float list and the entire column falls back to string serialisation — losing numeric type information and breaking aggregations.
2. None-first-value columns: When the first row value is null, pydruid.get_type(None) returns Type.STRING, labelling the column as STRING in the cursor description. Even though PyArrow correctly infers float64 from the remaining rows (no exception is raised), SupersetResultSet.data_type() was returning "STRING" because cursor description always took precedence — causing Superset to treat legitimately numeric columns as strings.
## Fix (gated behind a new PRESERVE_NUMERIC_COLUMNS_FOR_SPECIAL_FLOATS feature flag, default False):
1. In the pa.array() exception path, special-float strings are replaced with None before retrying PyArrow array construction, preserving the column as numeric with nulls in place of the special values.
2. In data_type(), when the cursor description says STRING but PyArrow inferred a more specific type (INT, FLOAT, DATETIME), PyArrow's inference wins — fixing the metadata case where no exception is raised.
The flag is off by default to avoid unintended type changes on existing deployments. Operators running Druid with nullable or special-float-heavy columns should enable it.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): preserve numeric column types when pydruid infers STRING from first-row value [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40677:
URL: https://github.com/apache/superset/pull/40677#issuecomment-4617720315
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): preserve numeric column types when pydruid infers STRING from first-row value [superset]
Posted by "madhushreeag (via GitHub)" <gi...@apache.org>.
madhushreeag commented on PR #40677:
URL: https://github.com/apache/superset/pull/40677#issuecomment-4617499215
/review
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): preserve numeric column types when pydruid infers STRING from first-row value [superset]
Posted by "madhushreeag (via GitHub)" <gi...@apache.org>.
madhushreeag commented on code in PR #40677:
URL: https://github.com/apache/superset/pull/40677#discussion_r3352418103
##########
superset/result_set.py:
##########
@@ -184,6 +184,20 @@ def __init__( # pylint: disable=too-many-locals # noqa: C901
TypeError, # this is super hackey,
# https://issues.apache.org/jira/browse/ARROW-7855
):
+ col_values = db_engine_spec.normalize_column_values(
+ array[column].tolist()
+ )
+ try:
+ pa_data.append(pa.array(col_values))
+ continue
+ except (
+ pa.lib.ArrowInvalid,
+ pa.lib.ArrowTypeError,
+ pa.lib.ArrowNotImplementedError,
+ ValueError,
+ TypeError,
+ ):
+ pass
Review Comment:
Deduped this code block
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset):add feature flag for preserving numeric columns for special floats [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40677:
URL: https://github.com/apache/superset/pull/40677#issuecomment-4599357578
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): preserve numeric column types when pydruid infers STRING from first-row value [superset]
Posted by "madhushreeag (via GitHub)" <gi...@apache.org>.
madhushreeag commented on code in PR #40677:
URL: https://github.com/apache/superset/pull/40677#discussion_r3358122912
##########
superset/result_set.py:
##########
@@ -174,16 +174,23 @@ def __init__( # pylint: disable=too-many-locals # noqa: C901
array = np.array(data, dtype=numpy_dtype)
for column in column_names:
- try:
- pa_data.append(pa.array(array[column].tolist()))
- except (
- pa.lib.ArrowInvalid,
- pa.lib.ArrowTypeError,
- pa.lib.ArrowNotImplementedError,
- ValueError,
- TypeError, # this is super hackey,
- # https://issues.apache.org/jira/browse/ARROW-7855
+ raw = array[column].tolist()
+ for col_values in (
+ raw,
+ db_engine_spec.normalize_column_values(raw),
Review Comment:
Good catch! The current code eagerly builds normalize_column_values(raw) even when the first pa.array() succeeds. Adding a requires_column_value_normalization flag to skip the second attempt entirely for engines that don’t need it.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): preserve numeric column types when pydruid infers STRING from first-row value [superset]
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro merged PR #40677:
URL: https://github.com/apache/superset/pull/40677
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): preserve numeric column types when pydruid infers STRING from first-row value [superset]
Posted by "madhushreeag (via GitHub)" <gi...@apache.org>.
madhushreeag commented on code in PR #40677:
URL: https://github.com/apache/superset/pull/40677#discussion_r3352480226
##########
superset/db_engine_specs/base.py:
##########
@@ -1304,6 +1304,38 @@ def get_datatype(cls, type_code: Any) -> str | None:
return type_code.upper()
return None
+ @classmethod
+ def normalize_column_values(cls, col_values: list[Any]) -> list[Any]:
+ """
+ Engine-specific hook to normalize column values before PyArrow conversion.
+
+ Called when the initial pa.array() conversion raises an exception, giving
+ the engine a chance to clean up values (e.g. replace sentinel strings with
+ None) before a second conversion attempt.
+
+ :param col_values: Raw Python values for one column
+ :return: Normalized values; return the input list unchanged by default
+ """
+ return col_values
+
+ @classmethod
+ def resolve_column_type(
+ cls, cursor_type: str | None, pa_mapped: str | None
+ ) -> str | None:
+ """
+ Choose the reported column type from the cursor description type and the
+ type inferred by PyArrow.
+
+ The default prefers the cursor description when available. Override in
+ engine specs where the cursor description is unreliable (e.g. pydruid
+ infers STRING from a None or special-float first row value).
+
+ :param cursor_type: Type string from the cursor description, or None
+ :param pa_mapped: Type string inferred by PyArrow, or None
+ :return: The type string to report for this column
+ """
+ return cursor_type or pa_mapped
Review Comment:
Added the tests in tests/unit_tests/db_engine_specs/test_druid.py to keep result_set.py decoupled from the individual engine peculiarities
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): preserve numeric column types when pydruid infers STRING from first-row value [superset]
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on code in PR #40677:
URL: https://github.com/apache/superset/pull/40677#discussion_r3359369588
##########
superset/result_set.py:
##########
@@ -174,8 +174,13 @@ def __init__( # pylint: disable=too-many-locals # noqa: C901
array = np.array(data, dtype=numpy_dtype)
for column in column_names:
+ raw = array[column].tolist()
+ if db_engine_spec.requires_column_value_normalization:
+ col_values = db_engine_spec.normalize_column_values(raw)
+ else:
+ col_values = raw
Review Comment:
Simplification (unless types change):
```suggestion
col_values = array[column].tolist()
if db_engine_spec.requires_column_value_normalization:
col_values = db_engine_spec.normalize_column_values(col_values)
```
While we're at it, that variable named `array` is pretty unpythonic and looks like a reserved keyword/built-in function or similar. Could we rename it to something more descriptive, like `columns` to at least be a plural of the later introduced `column`?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): preserve numeric column types when pydruid infers STRING from first-row value [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #40677:
URL: https://github.com/apache/superset/pull/40677#discussion_r3351446832
##########
superset/db_engine_specs/base.py:
##########
@@ -1304,6 +1304,38 @@ def get_datatype(cls, type_code: Any) -> str | None:
return type_code.upper()
return None
+ @classmethod
+ def normalize_column_values(cls, col_values: list[Any]) -> list[Any]:
+ """
+ Engine-specific hook to normalize column values before PyArrow conversion.
+
+ Called when the initial pa.array() conversion raises an exception, giving
+ the engine a chance to clean up values (e.g. replace sentinel strings with
+ None) before a second conversion attempt.
+
+ :param col_values: Raw Python values for one column
+ :return: Normalized values; return the input list unchanged by default
+ """
+ return col_values
+
+ @classmethod
+ def resolve_column_type(
+ cls, cursor_type: str | None, pa_mapped: str | None
+ ) -> str | None:
+ """
+ Choose the reported column type from the cursor description type and the
+ type inferred by PyArrow.
+
+ The default prefers the cursor description when available. Override in
+ engine specs where the cursor description is unreliable (e.g. pydruid
+ infers STRING from a None or special-float first row value).
+
+ :param cursor_type: Type string from the cursor description, or None
+ :param pa_mapped: Type string inferred by PyArrow, or None
+ :return: The type string to report for this column
+ """
+ return cursor_type or pa_mapped
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing unit tests for new methods</b></div>
<div id="fix">
New public methods `normalize_column_values` and `resolve_column_type` lack dedicated unit tests. Rule [11730] requires comprehensive unit tests for new features covering success paths and edge cases. Consider adding tests in `tests/unit_tests/result_set_test.py` or creating `tests/unit_tests/db_engine_specs/base_test.py`.
</div>
</div>
<small><i>Code Review Run #3f6c16</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): preserve numeric column types when pydruid infers STRING from first-row value [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40677:
URL: https://github.com/apache/superset/pull/40677#issuecomment-4616084332
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): preserve numeric column types when pydruid infers STRING from first-row value [superset]
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on code in PR #40677:
URL: https://github.com/apache/superset/pull/40677#discussion_r3351592195
##########
tests/unit_tests/result_set_test.py:
##########
@@ -264,3 +264,116 @@ def test_empty_column_names_do_not_rename_explicit_synthetic_names() -> None:
df = result_set.to_pandas_df()
assert list(df.columns) == ["_col_1", "_col_0"]
assert df.iloc[0].tolist() == [10, 20]
+
+
+# ---------------------------------------------------------------------------
+# DruidEngineSpec column normalization tests
+#
+# pydruid infers column types from the first row value, which causes two
+# related problems:
+#
+# Case 1 – Mixed IEEE special-float strings and numbers:
+# Druid cannot represent NaN/Infinity in JSON, so pydruid emits them as
+# the strings "NaN", "Infinity", or "-Infinity". When these appear in a
+# numeric column, pa.array() raises ArrowInvalid on the mixed str/float
+# list and the column falls back to string serialisation.
+#
+# Case 2 – None as the first value:
+# pydruid's get_type(None) returns Type.STRING, so any nullable numeric
+# column whose first row is null gets labelled STRING in the cursor
+# description. pa.array() succeeds (producing float64) but
+# data_type() used to return STRING because the cursor description won.
+#
+# DruidEngineSpec overrides normalize_column_values and resolve_column_type
+# to handle both cases. BaseEngineSpec preserves the original behaviour.
+# ---------------------------------------------------------------------------
+
+
+def test_druid_ieee_special_floats_preserved_as_numeric() -> None:
+ """
+ Case 1, DruidEngineSpec: columns that mix IEEE special-float strings with
+ real numbers must keep their numeric type (specials become null).
+ """
+ from superset.db_engine_specs.druid import DruidEngineSpec
+
+ data = [("NaN",), (1.5,), ("Infinity",), (2.3,), ("-Infinity",), (None,)]
+ description = [("metric", "STRING", None, None, None, None, None)]
+ result_set = SupersetResultSet(data, description, DruidEngineSpec) # type: ignore
+
+ col = result_set.columns[0]
+ assert col["type"] == "FLOAT"
+
+ df = result_set.to_pandas_df()
+ assert pd.isna(df["metric"].iloc[0]) # "NaN" → null
+ assert df["metric"].iloc[1] == 1.5
+ assert pd.isna(df["metric"].iloc[2]) # "Infinity" → null
+ assert df["metric"].iloc[3] == 2.3
+ assert pd.isna(df["metric"].iloc[4]) # "-Infinity" → null
+ assert pd.isna(df["metric"].iloc[5]) # None → null
Review Comment:
Optimally we'd place these tests in `tests/unit_tests/db_engine_specs/test_druid.py` to keep `result_set.py` as decoupled as possible from the individual engine peculiarities
##########
superset/result_set.py:
##########
@@ -184,6 +184,20 @@ def __init__( # pylint: disable=too-many-locals # noqa: C901
TypeError, # this is super hackey,
# https://issues.apache.org/jira/browse/ARROW-7855
):
+ col_values = db_engine_spec.normalize_column_values(
+ array[column].tolist()
+ )
+ try:
+ pa_data.append(pa.array(col_values))
+ continue
+ except (
+ pa.lib.ArrowInvalid,
+ pa.lib.ArrowTypeError,
+ pa.lib.ArrowNotImplementedError,
+ ValueError,
+ TypeError,
+ ):
+ pass
Review Comment:
It feels like this code is duplicating the try-except block on 177-186. Can we try to merge them so we only have a single block?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): preserve numeric column types when pydruid infers STRING from first-row value [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #40677:
URL: https://github.com/apache/superset/pull/40677#discussion_r3352480840
##########
superset/db_engine_specs/base.py:
##########
@@ -1304,6 +1304,38 @@ def get_datatype(cls, type_code: Any) -> str | None:
return type_code.upper()
return None
+ @classmethod
+ def normalize_column_values(cls, col_values: list[Any]) -> list[Any]:
+ """
+ Engine-specific hook to normalize column values before PyArrow conversion.
+
+ Called when the initial pa.array() conversion raises an exception, giving
+ the engine a chance to clean up values (e.g. replace sentinel strings with
+ None) before a second conversion attempt.
+
+ :param col_values: Raw Python values for one column
+ :return: Normalized values; return the input list unchanged by default
+ """
+ return col_values
+
+ @classmethod
+ def resolve_column_type(
+ cls, cursor_type: str | None, pa_mapped: str | None
+ ) -> str | None:
+ """
+ Choose the reported column type from the cursor description type and the
+ type inferred by PyArrow.
+
+ The default prefers the cursor description when available. Override in
+ engine specs where the cursor description is unreliable (e.g. pydruid
+ infers STRING from a None or special-float first row value).
+
+ :param cursor_type: Type string from the cursor description, or None
+ :param pa_mapped: Type string inferred by PyArrow, or None
+ :return: The type string to report for this column
+ """
+ return cursor_type or pa_mapped
Review Comment:
<!-- Bito Reply -->
The reviewer's suggestion to add unit tests for the new `normalize_column_values` and `resolve_column_type` methods is valid, as these are public methods introduced in the base engine specification. While you have already implemented tests in `tests/unit_tests/result_set_test.py`, adding dedicated unit tests in `tests/unit_tests/db_engine_specs/base_test.py` would further improve test coverage and maintainability by isolating the base class logic from the `SupersetResultSet` integration tests.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): preserve numeric column types when pydruid infers STRING from first-row value [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40677:
URL: https://github.com/apache/superset/pull/40677#issuecomment-4634607030
**Bito Automatic Review Skipped – PR Already Merged**<br/><br/>Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.<br/>No action is needed if you didn't intend to review it. To get a review, you can type `/review` in a comment and save it
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): preserve numeric column types when pydruid infers STRING from first-row value [superset]
Posted by "madhushreeag (via GitHub)" <gi...@apache.org>.
madhushreeag commented on code in PR #40677:
URL: https://github.com/apache/superset/pull/40677#discussion_r3343999516
##########
superset/result_set.py:
##########
@@ -184,6 +186,23 @@ def __init__( # pylint: disable=too-many-locals # noqa: C901
TypeError, # this is super hackey,
# https://issues.apache.org/jira/browse/ARROW-7855
):
+ col_values = array[column].tolist()
+ if is_feature_enabled("PRESERVE_NUMERIC_COLUMNS_FOR_SPECIAL_FLOATS"):
+ col_values = [
+ None if isinstance(v, str) and v in _FLOAT_SPECIAL else v
+ for v in col_values
+ ]
+ try:
+ pa_data.append(pa.array(col_values))
+ continue
Review Comment:
Outdated - Changed the logic
##########
superset/result_set.py:
##########
@@ -277,13 +296,26 @@ def type_generic(
def data_type(self, col_name: str, pa_dtype: pa.DataType) -> Optional[str]:
"""Given a pyarrow data type, Returns a generic database type"""
- if set_type := self._type_dict.get(col_name):
+ set_type = self._type_dict.get(col_name)
+ pa_mapped = self.convert_pa_dtype(pa_dtype)
+
+ # pydruid infers column types from the first row value, so a None or
+ # special-float-string first value causes the column to be labelled
+ # STRING even when the actual data is numeric. When the feature flag
+ # is enabled, prefer PyArrow's inferred type over a cursor-description
+ # STRING so that numeric columns are not misreported.
+ if (
+ is_feature_enabled("PRESERVE_NUMERIC_COLUMNS_FOR_SPECIAL_FLOATS")
+ and set_type == "STRING"
+ and pa_mapped is not None
+ and pa_mapped != "STRING"
+ ):
+ return pa_mapped
+
Review Comment:
Outdated - Changed the logic
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): preserve numeric column types when pydruid infers STRING from first-row value [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #40677:
URL: https://github.com/apache/superset/pull/40677#issuecomment-4614635833
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 2efd99bdc651b3f71037c09cd7294b1141eb295b |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6a20577a9d71c00008129ca6 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-40677--superset-docs-preview.netlify.app](https://deploy-preview-40677--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#q1YqKMrPLShRslJS0lFKKkrMS84AstMyK3QLilLLUvNKdPMSdVOKSjNTgPIpqQU5-ZWeKUAVZolGBqbm5omWKeaGyQZAYGFoZJmcaKZUCwA) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-40677--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): preserve numeric column types when pydruid infers STRING from first-row value [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40677:
URL: https://github.com/apache/superset/pull/40677#issuecomment-4617499842
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): preserve numeric column types when pydruid infers STRING from first-row value [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40677:
URL: https://github.com/apache/superset/pull/40677#issuecomment-4607605923
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): preserve numeric column types when pydruid infers STRING from first-row value [superset]
Posted by "madhushreeag (via GitHub)" <gi...@apache.org>.
madhushreeag commented on code in PR #40677:
URL: https://github.com/apache/superset/pull/40677#discussion_r3352412654
##########
superset/db_engine_specs/base.py:
##########
@@ -1304,6 +1304,38 @@ def get_datatype(cls, type_code: Any) -> str | None:
return type_code.upper()
return None
+ @classmethod
+ def normalize_column_values(cls, col_values: list[Any]) -> list[Any]:
+ """
+ Engine-specific hook to normalize column values before PyArrow conversion.
+
+ Called when the initial pa.array() conversion raises an exception, giving
+ the engine a chance to clean up values (e.g. replace sentinel strings with
+ None) before a second conversion attempt.
+
+ :param col_values: Raw Python values for one column
+ :return: Normalized values; return the input list unchanged by default
+ """
+ return col_values
+
+ @classmethod
+ def resolve_column_type(
+ cls, cursor_type: str | None, pa_mapped: str | None
+ ) -> str | None:
+ """
+ Choose the reported column type from the cursor description type and the
+ type inferred by PyArrow.
+
+ The default prefers the cursor description when available. Override in
+ engine specs where the cursor description is unreliable (e.g. pydruid
+ infers STRING from a None or special-float first row value).
+
+ :param cursor_type: Type string from the cursor description, or None
+ :param pa_mapped: Type string inferred by PyArrow, or None
+ :return: The type string to report for this column
+ """
+ return cursor_type or pa_mapped
Review Comment:
The methods are already tested indirectly via test_base_spec_ieee_special_floats_stringified and test_base_spec_none_first_value_reports_string_type in test_druid.py, but we added direct unit tests to satisfy it.
##########
superset/db_engine_specs/base.py:
##########
@@ -1304,6 +1304,38 @@ def get_datatype(cls, type_code: Any) -> str | None:
return type_code.upper()
return None
+ @classmethod
+ def normalize_column_values(cls, col_values: list[Any]) -> list[Any]:
+ """
+ Engine-specific hook to normalize column values before PyArrow conversion.
+
+ Called when the initial pa.array() conversion raises an exception, giving
+ the engine a chance to clean up values (e.g. replace sentinel strings with
+ None) before a second conversion attempt.
+
+ :param col_values: Raw Python values for one column
+ :return: Normalized values; return the input list unchanged by default
+ """
+ return col_values
+
+ @classmethod
+ def resolve_column_type(
+ cls, cursor_type: str | None, pa_mapped: str | None
+ ) -> str | None:
+ """
+ Choose the reported column type from the cursor description type and the
+ type inferred by PyArrow.
+
+ The default prefers the cursor description when available. Override in
+ engine specs where the cursor description is unreliable (e.g. pydruid
+ infers STRING from a None or special-float first row value).
+
+ :param cursor_type: Type string from the cursor description, or None
+ :param pa_mapped: Type string inferred by PyArrow, or None
+ :return: The type string to report for this column
+ """
+ return cursor_type or pa_mapped
Review Comment:
The methods are already tested indirectly via test_base_spec_ieee_special_floats_stringified and test_base_spec_none_first_value_reports_string_type in test_druid.py, but we added direct unit tests to satisfy this
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): preserve numeric column types when pydruid infers STRING from first-row value [superset]
Posted by "madhushreeag (via GitHub)" <gi...@apache.org>.
madhushreeag commented on code in PR #40677:
URL: https://github.com/apache/superset/pull/40677#discussion_r3352415139
##########
tests/unit_tests/result_set_test.py:
##########
@@ -264,3 +264,116 @@ def test_empty_column_names_do_not_rename_explicit_synthetic_names() -> None:
df = result_set.to_pandas_df()
assert list(df.columns) == ["_col_1", "_col_0"]
assert df.iloc[0].tolist() == [10, 20]
+
+
+# ---------------------------------------------------------------------------
+# DruidEngineSpec column normalization tests
+#
+# pydruid infers column types from the first row value, which causes two
+# related problems:
+#
+# Case 1 – Mixed IEEE special-float strings and numbers:
+# Druid cannot represent NaN/Infinity in JSON, so pydruid emits them as
+# the strings "NaN", "Infinity", or "-Infinity". When these appear in a
+# numeric column, pa.array() raises ArrowInvalid on the mixed str/float
+# list and the column falls back to string serialisation.
+#
+# Case 2 – None as the first value:
+# pydruid's get_type(None) returns Type.STRING, so any nullable numeric
+# column whose first row is null gets labelled STRING in the cursor
+# description. pa.array() succeeds (producing float64) but
+# data_type() used to return STRING because the cursor description won.
+#
+# DruidEngineSpec overrides normalize_column_values and resolve_column_type
+# to handle both cases. BaseEngineSpec preserves the original behaviour.
+# ---------------------------------------------------------------------------
+
+
+def test_druid_ieee_special_floats_preserved_as_numeric() -> None:
+ """
+ Case 1, DruidEngineSpec: columns that mix IEEE special-float strings with
+ real numbers must keep their numeric type (specials become null).
+ """
+ from superset.db_engine_specs.druid import DruidEngineSpec
+
+ data = [("NaN",), (1.5,), ("Infinity",), (2.3,), ("-Infinity",), (None,)]
+ description = [("metric", "STRING", None, None, None, None, None)]
+ result_set = SupersetResultSet(data, description, DruidEngineSpec) # type: ignore
+
+ col = result_set.columns[0]
+ assert col["type"] == "FLOAT"
+
+ df = result_set.to_pandas_df()
+ assert pd.isna(df["metric"].iloc[0]) # "NaN" → null
+ assert df["metric"].iloc[1] == 1.5
+ assert pd.isna(df["metric"].iloc[2]) # "Infinity" → null
+ assert df["metric"].iloc[3] == 2.3
+ assert pd.isna(df["metric"].iloc[4]) # "-Infinity" → null
+ assert pd.isna(df["metric"].iloc[5]) # None → null
Review Comment:
Moved the tests as suggested
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): preserve numeric column types when pydruid infers STRING from first-row value [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40677:
URL: https://github.com/apache/superset/pull/40677#issuecomment-4606478177
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): preserve numeric column types when pydruid infers STRING from first-row value [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on PR #40677:
URL: https://github.com/apache/superset/pull/40677#issuecomment-4615035648
@madhushreeag looks like you may need to fix CI. We can assign after things are green.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): preserve numeric column types when pydruid infers STRING from first-row value [superset]
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on code in PR #40677:
URL: https://github.com/apache/superset/pull/40677#discussion_r3357996034
##########
superset/result_set.py:
##########
@@ -174,16 +174,23 @@ def __init__( # pylint: disable=too-many-locals # noqa: C901
array = np.array(data, dtype=numpy_dtype)
for column in column_names:
- try:
- pa_data.append(pa.array(array[column].tolist()))
- except (
- pa.lib.ArrowInvalid,
- pa.lib.ArrowTypeError,
- pa.lib.ArrowNotImplementedError,
- ValueError,
- TypeError, # this is super hackey,
- # https://issues.apache.org/jira/browse/ARROW-7855
+ raw = array[column].tolist()
+ for col_values in (
+ raw,
+ db_engine_spec.normalize_column_values(raw),
Review Comment:
This will likely incur a perf hit. Maybe we can avoid this by using the old logic if an engine spec doesn't have a custom `normalize_column_values` method? Something like `BaseEngineSpec.requires_column_value_normalization = False` that would be set to `True` for Druid spec?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): add feature flag for preserving numeric columns for special floats [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #40677:
URL: https://github.com/apache/superset/pull/40677#discussion_r3339159928
##########
superset/result_set.py:
##########
@@ -184,6 +186,23 @@ def __init__( # pylint: disable=too-many-locals # noqa: C901
TypeError, # this is super hackey,
# https://issues.apache.org/jira/browse/ARROW-7855
):
+ col_values = array[column].tolist()
+ if is_feature_enabled("PRESERVE_NUMERIC_COLUMNS_FOR_SPECIAL_FLOATS"):
+ col_values = [
+ None if isinstance(v, str) and v in _FLOAT_SPECIAL else v
+ for v in col_values
+ ]
+ try:
+ pa_data.append(pa.array(col_values))
+ continue
Review Comment:
**Suggestion:** The special-float cleanup is applied globally to every engine whenever the feature flag is enabled, but this workaround is only valid for Druid/pydruid behavior. As written, enabling the flag for Druid can silently rewrite legitimate `"NaN"`/`"Infinity"` string values from other engines into nulls and change inferred Arrow types. Gate this branch by engine (for example, only when `db_engine_spec.engine == "druid"`) so non-Druid result sets are not mutated. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Non-Druid queries mis-serialize "NaN"/Infinity strings as nulls.
- ⚠️ SQL Lab and dashboards see unexpected numeric column types.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Enable the global feature flag so that
`superset.result_set.is_feature_enabled("PRESERVE_NUMERIC_COLUMNS_FOR_SPECIAL_FLOATS")`
returns True; this is the condition checked in the Arrow error path at
`superset/result_set.py:189-197`.
2. Execute a SQL statement against a non-Druid database (for example, a Postgres
`Database` whose engine spec is `PostgresEngineSpec` from
`superset/db_engine_specs/postgres.py`) through SQL Lab; this flows through
`SQLExecutor.execute()` to `execute_sql_with_cursor()` in
`superset/sql/execution/executor.py:17-25`.
3. In `execute_sql_with_cursor()`, when the cursor has a description, the code at
`superset/sql/execution/executor.py:79-87` constructs a `SupersetResultSet(rows,
description, database.db_engine_spec)`, passing the non-Druid `db_engine_spec` into
`SupersetResultSet.__init__` in `superset/result_set.py:135-143`.
4. If one of the result columns has mixed Python values like `["NaN", 1.5, "Infinity",
2.3, "-Infinity", None]` (the same pattern used in
`tests/unit_tests/result_set_test.py:45-58`), `pa.array(array[column].tolist())` at
`superset/result_set.py:178-181` raises `ArrowInvalid` and falls into the exception block
where `col_values` is set and, because the flag is enabled, the branch at
`superset/result_set.py:189-197` replaces any string `"NaN"`, `"Infinity"`, or
`"-Infinity"` with `None` for this non-Druid engine before re-running
`pa.array(col_values)`, silently converting legitimate string values to nulls and changing
the inferred Arrow type for that column.
```
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0e2b232739c34c809da0cc1b673c6ce6&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=0e2b232739c34c809da0cc1b673c6ce6&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/result_set.py
**Line:** 190:197
**Comment:**
*Logic Error: The special-float cleanup is applied globally to every engine whenever the feature flag is enabled, but this workaround is only valid for Druid/pydruid behavior. As written, enabling the flag for Druid can silently rewrite legitimate `"NaN"`/`"Infinity"` string values from other engines into nulls and change inferred Arrow types. Gate this branch by engine (for example, only when `db_engine_spec.engine == "druid"`) so non-Druid result sets are not mutated.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40677&comment_hash=c69f351253e32e792e0a870b3116fcb7201dc89e18fed4cd0eeef50a9589065a&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40677&comment_hash=c69f351253e32e792e0a870b3116fcb7201dc89e18fed4cd0eeef50a9589065a&reaction=dislike'>👎</a>
##########
superset/result_set.py:
##########
@@ -277,13 +296,26 @@ def type_generic(
def data_type(self, col_name: str, pa_dtype: pa.DataType) -> Optional[str]:
"""Given a pyarrow data type, Returns a generic database type"""
- if set_type := self._type_dict.get(col_name):
+ set_type = self._type_dict.get(col_name)
+ pa_mapped = self.convert_pa_dtype(pa_dtype)
+
+ # pydruid infers column types from the first row value, so a None or
+ # special-float-string first value causes the column to be labelled
+ # STRING even when the actual data is numeric. When the feature flag
+ # is enabled, prefer PyArrow's inferred type over a cursor-description
+ # STRING so that numeric columns are not misreported.
+ if (
+ is_feature_enabled("PRESERVE_NUMERIC_COLUMNS_FOR_SPECIAL_FLOATS")
+ and set_type == "STRING"
+ and pa_mapped is not None
+ and pa_mapped != "STRING"
+ ):
+ return pa_mapped
+
Review Comment:
**Suggestion:** The metadata override in `data_type()` also runs for all engines, so any backend that reports `STRING` in cursor metadata but returns typed Python values can be reclassified to numeric/datetime unexpectedly when the flag is enabled. This breaks type-contract consistency outside Druid. Restrict this override to Druid result sets (or another explicit pydruid-only condition) before preferring PyArrow type inference. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Non-Druid engines have metadata STRING columns retyped as numeric.
- ⚠️ SQL Lab column metadata inconsistent with database cursor description.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. With the feature flag enabled so that
`is_feature_enabled("PRESERVE_NUMERIC_COLUMNS_FOR_SPECIAL_FLOATS")` is True, construct a
`SupersetResultSet` as done in
`tests/unit_tests/result_set_test.py:test_druid_none_first_value_reports_numeric_type_with_flag_enabled`
(lines 87-104): pass `data = [(None,), (1.5,), (2.3,), (None,), (4.7,)]`, a cursor
`description = [("metric", "STRING", None, None, None, None, None)]`, and a non-Druid
engine spec such as `PostgresEngineSpec` from `superset/db_engine_specs/postgres.py` into
`SupersetResultSet(data, description, PostgresEngineSpec)`.
2. Inside `SupersetResultSet.__init__` (`superset/result_set.py:135-251`), `_type_dict` is
populated at `superset/result_set.py:243-250` using
`db_engine_spec.get_datatype(deduped_cursor_desc[i][1])`, so for this column `set_type` in
`data_type()` becomes `"STRING"` based solely on the cursor metadata from the non-Druid
engine.
3. When SQL Lab or the async executor needs column metadata, `SupersetResultSet.columns`
at `superset/result_set.py:73-88` iterates over the Arrow schema and calls
`self.data_type(col.name, col.type)`; `data_type()` (lines 38-59) computes `pa_mapped =
self.convert_pa_dtype(pa_dtype)`, which returns `"FLOAT"` for the `float64` Arrow type
inferred from the actual data.
4. Because the override condition at `superset/result_set.py:48-54` currently checks only
the feature flag and `set_type == "STRING"` (without verifying that
`self.db_engine_spec.engine` is `"druid"`), the branch returns `pa_mapped` (`"FLOAT"`) for
this non-Druid engine, so `_serialize_result_set()` in
`superset/sql/execution/celery_task.py:111-122` returns `columns = result_set.columns`
with a `"FLOAT"` type even though the engine's cursor description declared `"STRING"`,
causing type metadata for non-Druid backends to be reclassified unexpectedly whenever this
flag is on.
```
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d654c945036941bd97cb24caefd2f5a7&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=d654c945036941bd97cb24caefd2f5a7&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/result_set.py
**Line:** 308:314
**Comment:**
*Api Mismatch: The metadata override in `data_type()` also runs for all engines, so any backend that reports `STRING` in cursor metadata but returns typed Python values can be reclassified to numeric/datetime unexpectedly when the flag is enabled. This breaks type-contract consistency outside Druid. Restrict this override to Druid result sets (or another explicit pydruid-only condition) before preferring PyArrow type inference.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40677&comment_hash=96303c073287ecf142a80b2093d0e10558a4d47dc845f5f4af39b3ead73b956e&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40677&comment_hash=96303c073287ecf142a80b2093d0e10558a4d47dc845f5f4af39b3ead73b956e&reaction=dislike'>👎</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): add feature flag for preserving numeric columns for special floats [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #40677:
URL: https://github.com/apache/superset/pull/40677#issuecomment-4599442730
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/40677?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `43.75000%` with `9 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 63.17%. Comparing base ([`2abbb64`](https://app.codecov.io/gh/apache/superset/commit/2abbb64e6bcae04e9748893e0a11ff574a3680b2?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`c3ea778`](https://app.codecov.io/gh/apache/superset/commit/c3ea7785a962335bc3b45c383cc02a74d535df0d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/40677?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/result\_set.py](https://app.codecov.io/gh/apache/superset/pull/40677?src=pr&el=tree&filepath=superset%2Fresult_set.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvcmVzdWx0X3NldC5weQ==) | 43.75% | [7 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/superset/pull/40677?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #40677 +/- ##
==========================================
- Coverage 63.97% 63.17% -0.80%
==========================================
Files 2658 2658
Lines 143094 143106 +12
Branches 32906 32907 +1
==========================================
- Hits 91538 90403 -1135
- Misses 49991 51130 +1139
- Partials 1565 1573 +8
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/40677/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/40677/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.76% <25.00%> (+<0.01%)` | :arrow_up: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/40677/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/40677/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/40677/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `41.36% <25.00%> (+<0.01%)` | :arrow_up: |
| [python](https://app.codecov.io/gh/apache/superset/pull/40677/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `58.24% <43.75%> (-1.72%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/40677/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `58.12% <43.75%> (-0.01%)` | :arrow_down: |
| [unit](https://app.codecov.io/gh/apache/superset/pull/40677/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/40677?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): preserve numeric column types when pydruid infers STRING from first-row value [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #40677:
URL: https://github.com/apache/superset/pull/40677#discussion_r3352413715
##########
superset/db_engine_specs/base.py:
##########
@@ -1304,6 +1304,38 @@ def get_datatype(cls, type_code: Any) -> str | None:
return type_code.upper()
return None
+ @classmethod
+ def normalize_column_values(cls, col_values: list[Any]) -> list[Any]:
+ """
+ Engine-specific hook to normalize column values before PyArrow conversion.
+
+ Called when the initial pa.array() conversion raises an exception, giving
+ the engine a chance to clean up values (e.g. replace sentinel strings with
+ None) before a second conversion attempt.
+
+ :param col_values: Raw Python values for one column
+ :return: Normalized values; return the input list unchanged by default
+ """
+ return col_values
+
+ @classmethod
+ def resolve_column_type(
+ cls, cursor_type: str | None, pa_mapped: str | None
+ ) -> str | None:
+ """
+ Choose the reported column type from the cursor description type and the
+ type inferred by PyArrow.
+
+ The default prefers the cursor description when available. Override in
+ engine specs where the cursor description is unreliable (e.g. pydruid
+ infers STRING from a None or special-float first row value).
+
+ :param cursor_type: Type string from the cursor description, or None
+ :param pa_mapped: Type string inferred by PyArrow, or None
+ :return: The type string to report for this column
+ """
+ return cursor_type or pa_mapped
Review Comment:
<!-- Bito Reply -->
The reviewer's suggestion to add unit tests for the new `normalize_column_values` and `resolve_column_type` methods is valid, as these are public methods introduced in the base engine specification. While you have already implemented tests in `tests/unit_tests/result_set_test.py` (as seen in the PR comments), adding dedicated unit tests in `tests/unit_tests/db_engine_specs/base_test.py` would further improve test coverage and maintainability by isolating the base class logic from the `SupersetResultSet` integration tests.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): preserve numeric column types when pydruid infers STRING from first-row value [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #40677:
URL: https://github.com/apache/superset/pull/40677#issuecomment-4626305575
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): preserve numeric column types when pydruid infers STRING from first-row value [superset]
Posted by "madhushreeag (via GitHub)" <gi...@apache.org>.
madhushreeag commented on code in PR #40677:
URL: https://github.com/apache/superset/pull/40677#discussion_r3363858125
##########
superset/result_set.py:
##########
@@ -174,8 +174,13 @@ def __init__( # pylint: disable=too-many-locals # noqa: C901
array = np.array(data, dtype=numpy_dtype)
for column in column_names:
+ raw = array[column].tolist()
+ if db_engine_spec.requires_column_value_normalization:
+ col_values = db_engine_spec.normalize_column_values(raw)
+ else:
+ col_values = raw
Review Comment:
Adjusted the code
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(dataset): preserve numeric column types when pydruid infers STRING from first-row value [superset]
Posted by "madhushreeag (via GitHub)" <gi...@apache.org>.
madhushreeag commented on PR #40677:
URL: https://github.com/apache/superset/pull/40677#issuecomment-4606477263
/review
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org