Conversation
sansim26
left a comment
There was a problem hiding this comment.
LGTM, one suggestion is to provide links to documentation for data reading methods used may not always retrieve data in a consistent order
zzjas
left a comment
There was a problem hiding this comment.
It's your call to spend more time addressing the comments or not, but we approve you to proceed to open a real PR. Once you open a real PR, please mark this tentative PR as Opened in your tentative_pr.csv file and also raise a PR to IDoFT marking this as Opened. Thanks!
| try { | ||
| JSONAssert.assertEquals(expectedJson, DEFAULT_JSON_WRITER.writeValueAsString(sampledAsString.get(0).getRawValues()), false); | ||
| JSONAssert.assertEquals(expectedJson, DEFAULT_JSON_WRITER.writeValueAsString(sampledAsDate.get(0).getRawValues()), false); | ||
| } catch (JSONException jse) { |
There was a problem hiding this comment.
Is the JSONException type necessary here? Developers might be reluctant to add a new dependency for printing an error message in a test.
219sansim
left a comment
There was a problem hiding this comment.
See if you can remove dependencies that you included in the code.
|
Have you opened a real PR for this? If so, please mark this as "Opened" in your |
Description
Fixed flaky test testDateHandling in TimestampsParquetReaderTest.java with JSONAssert
Location:
extensions-core/parquet-extensionsFilename:
TimestampsParquetReaderTest.javaTest:
testDateHandlingTest Overview:
In the
TimestampsParquetReaderTest.testDateHandlingtest, we assess how timestamps are handled in Parquet files, focusing on date values stored asstringsanddate objects. The test checks the consistency of timestamp conversion and verifies that the JSON representations of these timestamps match the expected values.Reason of flakiness:
However, a potential problem arises because the data reading methods used
createReader()does not always retrieve data in a consistent order, possibly introducing randomness. This can lead to different JSON representations of the same data during test runs.Changes:
Imported
JSONAssertandJSONExceptionpackages, and changed from using standard assertion methodAssertto usingJSONAssertwhen comparing two resultssampledAsStringandsampledAsDatewithexpectedJson.Add
org.jsonandorg.skyscreamerdependencies topom.xmlfile in/parquet-extensions.To address this issue, the PR proposes changing the comparison approach from a standard assertion method
Assertto usingJSONAssert.JSONAssertmethod allows for a comparison of data equality without considering the order in which the data is retrieved. This change ensures more reliable and consistent test results across different runs, regardless of variations in data retrieval order.Release note
Fixed:
TimestampsParquetReaderTest.testDateHandlingtest no longer fail when running with the NonDex tool.This PR has: