Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: Added reference parquet files and tests for ObjectCodec implementations #6207

Merged
merged 2 commits into from
Oct 16, 2024

Conversation

malhotrashivam
Copy link
Contributor

Closes #5767

@malhotrashivam malhotrashivam added test parquet Related to the Parquet integration NoDocumentationNeeded NoReleaseNotesNeeded No release notes are needed. labels Oct 16, 2024
@malhotrashivam malhotrashivam added this to the 0.37.0 milestone Oct 16, 2024
@malhotrashivam malhotrashivam requested a review from rcaudy October 16, 2024 16:24
@malhotrashivam malhotrashivam self-assigned this Oct 16, 2024
@malhotrashivam malhotrashivam changed the title Added reference parquet files and tests for ObjectCodec implementations test: Added reference parquet files and tests for ObjectCodec implementations Oct 16, 2024
Copy link
Member

@rcaudy rcaudy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Please add coverage for more null cases (easy just to have a null for every type), negative values in BigDecimalCodec, compact values in LocalTimeCodec, and ExternalizableCodec.

@malhotrashivam
Copy link
Contributor Author

Looks good. Please add coverage for more null cases (easy just to have a null for every type), negative values in BigDecimalCodec, compact values in LocalTimeCodec, and ExternalizableCodec.

  • Added null values in columns, but they still don't cover the null cases in codec because these values are encoded as parquet nulls and not as null objects. Therefore, we don't hit the null case in codec.
  • Added negative values in BigDecimalCodec and compact values in LocalTimeCodec
  • ExternalizableCodec::decode was already covered in TestCodecColumns.

@rcaudy
Copy link
Member

rcaudy commented Oct 16, 2024

Looks good. Please add coverage for more null cases (easy just to have a null for every type), negative values in BigDecimalCodec, compact values in LocalTimeCodec, and ExternalizableCodec.

  • Added null values in columns, but they still don't cover the null cases in codec because these values are encoded as parquet nulls and not as null objects. Therefore, we don't hit the null case in codec.
  • Added negative values in BigDecimalCodec and compact values in LocalTimeCodec
  • ExternalizableCodec::decode was already covered in TestCodecColumns.
  • Our legacy Parquet behavior didn't properly encode nulls, right?
  • If ExternalizableCodec was covered, why didn't it show up on my coverage report? My coverage report was lying.

@malhotrashivam
Copy link
Contributor Author

malhotrashivam commented Oct 16, 2024

I tried regenerating data before my change #4191 but that didn't hit the null case in codec too because that issue was only in the primitive types where, for example, we stored null value as NULL_CHAR or NULL_INT instead of parquet nulls.

@malhotrashivam malhotrashivam merged commit 9d5006f into deephaven:main Oct 16, 2024
17 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Oct 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
NoDocumentationNeeded NoReleaseNotesNeeded No release notes are needed. parquet Related to the Parquet integration test
Projects
None yet
2 participants