You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "luizotavio32 (via GitHub)" <gi...@apache.org> on 2025/10/09 18:59:44 UTC
[PR] fix(csv upload): Correctly casting to string numbers with floating points (e+) [superset]
luizotavio32 opened a new pull request, #35586:
URL: https://github.com/apache/superset/pull/35586
### SUMMARY
Improved CSV reader type casting by allowing pandas to handle `str`, `object`, and `string` types directly instead of forcing all type conversions through custom casting logic.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
Upload a .csv with the following column and values, define the typing as {"id":"str"}
```
id
1439403621518935563
42286989
1413660691875593351
8.26839E+17
```
The dataset created should have the exact same values as in the csv
--
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(csv upload): Correctly casting to string numbers with floating points (e+) [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe merged PR #35586:
URL: https://github.com/apache/superset/pull/35586
--
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(csv upload): Correctly casting to string numbers with floating points (e+) [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #35586:
URL: https://github.com/apache/superset/pull/35586#discussion_r2417773652
##########
superset/commands/database/uploaders/csv_reader.py:
##########
@@ -326,6 +326,26 @@ def _cast_column_types(
CSVReader._cast_single_column(df, column, dtype, kwargs)
return df
+ @staticmethod
+ def _split_types(types: dict[str, str]) -> tuple[dict[str, str], dict[str, str]]:
+ """
+ Split column data types into custom and pandas-native types.
+
+ :param types: Dictionary mapping column names to data types
+ :return: Tuple of (custom_types, pandas_types) dictionaries
+ """
+ pandas_types = {
+ col: dtype
+ for col, dtype in types.items()
+ if dtype in ("str", "object", "string")
+ }
Review Comment:
Understood. The focus on text-related types for automatic pandas casting aligns with your implementation. This approach allows for more controlled handling of numeric 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: 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(csv upload): Correctly casting to string numbers with floating points (e+) [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #35586:
URL: https://github.com/apache/superset/pull/35586#issuecomment-3387172052
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/35586?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 `0%` with `13 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 50.72%. Comparing base ([`76d897e`](https://app.codecov.io/gh/apache/superset/commit/76d897eaa2f9e137102bc194c2e3109c29d0348f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`71a8996`](https://app.codecov.io/gh/apache/superset/commit/71a8996025310f0d799568c99fb7893886e57ce8?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 2695 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/35586?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/commands/database/uploaders/csv\_reader.py](https://app.codecov.io/gh/apache/superset/pull/35586?src=pr&el=tree&filepath=superset%2Fcommands%2Fdatabase%2Fuploaders%2Fcsv_reader.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvY29tbWFuZHMvZGF0YWJhc2UvdXBsb2FkZXJzL2Nzdl9yZWFkZXIucHk=) | 0.00% | [13 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/35586?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 #35586 +/- ##
==========================================
- Coverage 60.48% 50.72% -9.77%
==========================================
Files 1931 589 -1342
Lines 76236 43608 -32628
Branches 8568 4721 -3847
==========================================
- Hits 46114 22119 -23995
+ Misses 28017 20868 -7149
+ Partials 2105 621 -1484
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/35586/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/35586/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.26% <0.00%> (-2.90%)` | :arrow_down: |
| [javascript](https://app.codecov.io/gh/apache/superset/pull/35586/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/35586/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `49.97% <0.00%> (-3.84%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/35586/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `50.71% <0.00%> (-12.79%)` | :arrow_down: |
| [unit](https://app.codecov.io/gh/apache/superset/pull/35586/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ΓΈ> (+42.36%)` | :arrow_up: |
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/35586?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(csv upload): Correctly casting to string numbers with floating points (e+) [superset]
Posted by "luizotavio32 (via GitHub)" <gi...@apache.org>.
luizotavio32 commented on code in PR #35586:
URL: https://github.com/apache/superset/pull/35586#discussion_r2417772936
##########
superset/commands/database/uploaders/csv_reader.py:
##########
@@ -326,6 +326,26 @@ def _cast_column_types(
CSVReader._cast_single_column(df, column, dtype, kwargs)
return df
+ @staticmethod
+ def _split_types(types: dict[str, str]) -> tuple[dict[str, str], dict[str, str]]:
+ """
+ Split column data types into custom and pandas-native types.
+
+ :param types: Dictionary mapping column names to data types
+ :return: Tuple of (custom_types, pandas_types) dictionaries
+ """
+ pandas_types = {
+ col: dtype
+ for col, dtype in types.items()
+ if dtype in ("str", "object", "string")
+ }
Review Comment:
The idea is to only choose text related types to let pandas do the casting automatically
--
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(csv upload): Correctly casting to string numbers with floating points (e+) [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #35586:
URL: https://github.com/apache/superset/pull/35586#discussion_r2417710259
##########
superset/commands/database/uploaders/csv_reader.py:
##########
@@ -357,7 +357,28 @@ def _read_csv( # noqa: C901
kwargs["low_memory"] = False
try:
- types = kwargs.pop("dtype", None)
+ # Only pop types that are not str or object
+ original_types = kwargs.get("dtype", None)
+ types = None
+ if original_types:
+ # keep str/object in kwargs, extract others for custom casting
+ pandas_types = {}
+ custom_types = {}
Review Comment:
### Missing type handling encapsulation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Type handling logic is embedded within the _read_csv method instead of being encapsulated in a dedicated type handling class or method.
###### Why this matters
Mixing type handling with CSV reading logic reduces code reusability and makes it harder to modify type handling behavior independently.
###### Suggested change β *Feature Preview*
Create a dedicated TypeHandler class:
```python
class CSVTypeHandler:
def split_types(self, types: dict[str, str]) -> tuple[dict[str, str], dict[str, str]]:
pandas_types = {col: dtype for col, dtype in types.items() if dtype in ('str', 'object', 'string')}
custom_types = {col: dtype for col, dtype in types.items() if dtype not in ('str', 'object', 'string')}
return pandas_types, custom_types
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a736bb38-bf95-47ff-9369-3b100dbe7f46/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a736bb38-bf95-47ff-9369-3b100dbe7f46?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a736bb38-bf95-47ff-9369-3b100dbe7f46?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a736bb38-bf95-47ff-9369-3b100dbe7f46?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a736bb38-bf95-47ff-9369-3b100dbe7f46)
</details>
<sub>
π¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:f8dd1a16-addf-4635-8e47-17617b73a291 -->
[](f8dd1a16-addf-4635-8e47-17617b73a291)
--
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(csv upload): Correctly casting to string numbers with floating points (e+) [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #35586:
URL: https://github.com/apache/superset/pull/35586#discussion_r2417765814
##########
superset/commands/database/uploaders/csv_reader.py:
##########
@@ -326,6 +326,26 @@ def _cast_column_types(
CSVReader._cast_single_column(df, column, dtype, kwargs)
return df
+ @staticmethod
+ def _split_types(types: dict[str, str]) -> tuple[dict[str, str], dict[str, str]]:
+ """
+ Split column data types into custom and pandas-native types.
+
+ :param types: Dictionary mapping column names to data types
+ :return: Tuple of (custom_types, pandas_types) dictionaries
+ """
+ pandas_types = {
+ col: dtype
+ for col, dtype in types.items()
+ if dtype in ("str", "object", "string")
+ }
Review Comment:
### Incomplete pandas-native type classification <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The _split_types method incorrectly categorizes pandas-native types by only including string-like types ('str', 'object', 'string'), but excludes numeric types like 'int64', 'float64', 'int32', 'float32' which are also pandas-native types that should be handled by pandas directly.
###### Why this matters
This will cause all numeric types to be treated as custom types requiring manual casting, which defeats the purpose of leveraging pandas' native dtype handling for better performance and reliability. Scientific notation numbers (e+) that could be handled natively by pandas will unnecessarily go through custom casting.
###### Suggested change β *Feature Preview*
Include numeric types in the pandas_types classification:
```python
pandas_native_types = {"str", "object", "string", "int64", "int32", "float64", "float32", "bool"}
pandas_types = {
col: dtype
for col, dtype in types.items()
if dtype in pandas_native_types
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/76f8c6a1-504a-4312-b6c1-1dbe7ba2047a/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/76f8c6a1-504a-4312-b6c1-1dbe7ba2047a?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/76f8c6a1-504a-4312-b6c1-1dbe7ba2047a?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/76f8c6a1-504a-4312-b6c1-1dbe7ba2047a?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/76f8c6a1-504a-4312-b6c1-1dbe7ba2047a)
</details>
<sub>
π¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:8b26650e-8a28-401a-8de3-ed1ebaefd04e -->
[](8b26650e-8a28-401a-8de3-ed1ebaefd04e)
--
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