fix(connect): support configurable decimal separator in CSV parser - #4878
fix(connect): support configurable decimal separator in CSV parser#4878manish9363 wants to merge 3 commits into
Conversation
CsvFormat/CsvParser did not recognize numeric values using ',' as the decimal separator, so they were treated as strings (issue apache#530). - Add locale-aware overloads to DatatypeUtils (getTypeClass, getXsdDatatype, convertValue) that accept a decimal separator and normalize ',' to '.' before parsing. Existing overloads default to '.', so behavior is unchanged for current callers. - Normalization is guarded: it is skipped when the separator occurs more than once (e.g. grouping separators like 1,000,000) to avoid corrupting values. - Expose a 'Decimal separator' option on the CSV parser and thread it through. Closes apache#530
github-actions
Bot
added
java
|
Hi @manish9363, thanks a lot for the PR! Could you please confirm that you reproduced the issue in a running StreamPipes instance and tested the fix end-to-end locally? It would also be great if you could add a few screenshots showing the issue before the fix and the working behavior after the fix. This helps us verify that the change was tested in a real running system. |
Yes, I reproduced the issue and tested the fix end-to-end in a running StreamPipes instance (Docker Compose). Screenshots below. Setup: I used a CSV with ; as the column delimiter and , as the decimal separator: sensor;temperature;humidity Before (unpatched): In Connect → File Stream → CSV, temperature and humidity are detected as STRING, and the event preview keeps them as text ("temperature": "3,14"). After (with this fix): The CSV parser now exposes a "Decimal separator" option. Setting it to , makes temperature and humidity correctly detected as FLOAT, and the preview shows real numbers ("temperature": 3.14). sensor stays a STRING as expected. |
|
@tenthe : Let me know if you need anything else |
There was a problem hiding this comment.
@manish9363 Thank you for your contributions. The changes look good to me. Just 2 things that need to be fixed:
- The Cypress tests that use the file stream adapter like for example
fileStream.spec.tsneed to add the new input field to run successfully - Values with a period
.like 100.000 should not get parsed as float when a different decimal separator is selected. I think that's the better design because a period could also be used as a grouping separator for large numbers
Values using a separator other than the configured one are now kept as strings. For example "100.000" is no longer parsed as a float when the decimal separator is set to ",". Same for ambiguous cases with more than one separator. Also updated the Cypress connect tests to fill the new decimal separator field, since it is required and was blocking the file stream adapter flow.
@SvenO3 Thanks for the review. I have
Verified locally in a running instance - 100.000 stays a string and 3,14 |
|
Hi @manish9363, Did you notice that the following Cypress smoke tests are currently failing?
Maybe you can also run the full Cypress test suite locally to make sure the new option does not break any existing tests? If you need any help, just let us know. |
3 participants
Closes #530
CSV values using
,as the decimal separator were detected as strings instead of numbers. This adds a "Decimal separator" option to the CSV parser and makesDatatypeUtilsnormalize the configured separator before numeric detection.Behavior is additive and backward-compatible: existing
DatatypeUtilsoverloads default to., and normalization is skipped for ambiguous values (e.g.1,000,000) to avoid corrupting grouped numbers.Tested: new unit tests in
DatatypeUtilsTest(comma decimals + guard cases) andCsvParserTest(;delimiter with,decimals).mvn testgreen for both affected modules with 0 checkstyle violations.Open question for maintainers: I implemented lenient behavior (configuring
,still also accepts.-values). Happy to switch to strict locale honoring if preferred.