[DCP - SigFigs] Preserve SigFigs through Dataflow. Stop converting to double/int - #723
[DCP - SigFigs] Preserve SigFigs through Dataflow. Stop converting to double/int#723gmechali wants to merge 5 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the ingestion pipeline to prefer raw string/text representations of observation values over double/float representations. This change preserves significant figures and avoids floating-point precision issues or scientific notation formatting. It updates GraphReader, GraphUtils, and StatChecker to use text fields, removes the unused nodeDoubleValue helper, and updates corresponding unit tests to verify significant figure preservation. There are no review comments, so I have no feedback to provide.
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| CodeStyle | 2 minor |
🟢 Metrics 0 complexity
Metric Results Complexity 0
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors observation value handling to use raw string representations instead of floating-point numbers, aiming to preserve significant figures and prevent precision loss. Key changes include removing the number field from McfStatVarObsSeries.StatVarObs in favor of text, and updating associated reader, utility, and checker classes. The review feedback highlights two critical issues: first, reserving the number field in the proto definition breaks backward compatibility with legacy serialized data, so keeping it as a deprecated optional field is recommended; second, a non-atomic check-and-act pattern in StatChecker introduces a race condition under concurrent execution, which should be resolved using putIfAbsent.
| "lineNumber": "3" | ||
| }, | ||
| "userMessage": "Found nodes with different values for the same StatVarObservation :: observationAbout: 'geoId/01', variableMeasured: 'Some_StatVar', observationDate: '2020-01-01', value1: 1.0, value2: 2.0", | ||
| "userMessage": "Found nodes with different values for the same StatVarObservation :: observationAbout: 'geoId/01', variableMeasured: 'Some_StatVar', observationDate: '2020-01-01', value1: 1, value2: 2", |
There was a problem hiding this comment.
Fyi the reason we are now removing the "1.0" and instead showing "1" isnt' about a loss of sigfigs but actually preservation of sigfigs.
We used to convert the 1 to a float making it 1.0. Now we preserve the value.
| "location": { | ||
| "file": "covid.mcf", | ||
| "lineNumber": "17" | ||
| "entries": [ |
There was a problem hiding this comment.
Note the only diffs here are the same as the other two report.jsons (preserving sigfigs). This is just a formatting diff
|
FYI - this fix works in conjunction with #724 @vish-cs Can you pls take a look at both of these PRs and ensure that we're okay from a base dc perspective as well? Im specifically interested in your opinion on the "batch job", our "ET" part. I m not super familiar with it and want to make sure that you're not expecting dataflow to process those as numbers. The current PR deprecates using numbers on the MCF Obs object. Side note: we are likely to see some significant diffs in data as we begin to ingest more data with this version of dataflow. It should only ever be differences to the observation values to ensure the sigfigs are preserved. It should be simple to verify, but something we ll all need to keep an eye out for. |
| string text = 3; | ||
| } | ||
| // Value of the observation in text format. | ||
| optional string text = 3; |
There was a problem hiding this comment.
nit: consider just naming this value
Totally understand if it's not worth a bunch of extra overhead though
| if (this.svObValues.containsKey(fp) && !this.svObValues.get(fp).equals(val)) { | ||
| String val = McfUtil.getPropVal(node, Vocabulary.VALUE); | ||
| String existingVal = this.svObValues.putIfAbsent(fp, val); | ||
| if (existingVal != null && !existingVal.equals(val)) { |
There was a problem hiding this comment.
acking that comparing raw strings here means duplicate observations with different precision formatting (like 12.5 vs 12.50) will now flag as an error.
This makes sense to enforce strict SigFig consistency and eliminates the old float truncation issues.
|
Thanks Gabe. Is there a doc/read where the severity of issue is discussed. There were discussions of saving numeric values in spanner for query time benefits (inline arithmetic operations). Would be better to evaluate before we make wider change. |
3 participants
Summary
Refactors observation value handling across the Dataflow ingestion pipeline and Java utilities to use exact raw string representations, eliminating precision loss and significant figure degradation caused by floating-point conversions.
Mcf.proto): Reserved field 2 (number) inMcfStatVarObsSeries.StatVarObsand streamlined observation value representation tooptional string text = 3.GraphReader.java): UpdatedtoObservationto deserializeObservation.valuedirectly usingobs.getText().GraphUtils.java): PopulateMcfStatVarObsSerieswith raw string text; removed dead numeric-conversion helper methods.StatChecker.java):svObValuesasStringinConcurrentMapto prevent float-rounding collisions on high-precision inputs.putIfAbsentincheckSvObsValueInconsistencyto ensure thread-safety and avoid race conditions under concurrent execution.tool/): Updatedreport.jsontest expectations indatacommons-import-toolto reflect exact raw string messages.Testing
util/,pipeline/util/,tool/).mvn fmt:check.ingestion-gabe-test.json).samples/sigfig_testJSON-LD shards into live Spanner database (gmechali-latest-dc-db).Observationtable and confirmed 100% of observations match exact raw input strings (12.50,12.5000,0.0000001230,100000000000000001,-15.00).