Skip to content

Fix for export projection and null data excluded bugs #2583

New issue

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

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

Already on GitHub? # to your account

Merged
merged 5 commits into from
May 8, 2025

Conversation

inv-jishnu
Copy link
Contributor

@inv-jishnu inv-jishnu commented Apr 6, 2025

Description

This PR is to fix 2 bugs found during testing, The issues were raised and fixed by @thongdk8 (Thank you). I have added the fix to this branch along with more unit tests to check and confirm the fix

  • The import data file is empty when projections are not specified.
  • For Json and JsonLines format, if a column values is null it excluded from export file.

Related issues and/or PRs

NA

Changes made

The following issues are fixed

  • The import data file is empty when projections are not specified (Should include all columns).
    For example, if projection argument was not specified with required columns, the generated import file would be empty.
  • For Json and JsonLines format, if a column values is null it excluded from export file (Should include columns with null data).
    For example, if a value of a column that is to be imported is null for a particular row of data, the column is not added for that data in the import data file. Let say table sample has columns a, b, c, consider a to be the partition key. Let say for a=1, the respective value of b is null, hence the entry for a=1, in the import file b will be excluded and only a and c will be added to it.

Checklist

The following is a best-effort checklist. If any items in this checklist are not applicable to this PR or are dependent on other, unmerged PRs, please still mark the checkboxes after you have read and understood each item.

  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation to reflect the changes.
  • I have considered whether similar issues could occur in other products, components, or modules if this PR is for bug fixes.
  • Any remaining open issues linked to this PR are documented and up-to-date (Jira, GitHub, etc.).
  • Tests (unit, integration, etc.) have been added for the changes.
  • My changes generate no new warnings.
  • Any dependent changes in other PRs have been merged and published.

Additional notes (optional)

This is a bug fix PR for data loader export part

Release notes

NA

@inv-jishnu inv-jishnu self-assigned this Apr 6, 2025
@inv-jishnu inv-jishnu marked this pull request as draft April 6, 2025 16:00
@inv-jishnu inv-jishnu changed the title Fix export projection and null data excluded bug fixes Fix for export projection and null data excluded bugs May 7, 2025
@ypeckstadt ypeckstadt marked this pull request as ready for review May 7, 2025 05:20
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes two export bugs by ensuring that (1) when projections are not specified, all columns are exported and (2) null column values are correctly included in the output across JSON, JSONLines, and CSV formats.

  • Added tests for partial projections with and without metadata.
  • Updated producer tasks to correctly handle cases when no projections are provided and when a column value is null.
  • Enhanced UnitTestUtils to support partial output data for more granular testing.

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file
File Description
data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataexport/producer/JsonProducerTaskTest.java Added tests covering both projection scenarios and metadata inclusion/exclusion for JSON export.
data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataexport/producer/JsonLineProducerTaskTest.java Added tests for JSONLines export with various projection and metadata combinations.
data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataexport/producer/CsvProducerTaskTest.java Added tests for CSV export with similar projection and metadata scenarios, though test method names reference JSONLine incorrectly.
data-loader/core/src/test/java/com/scalar/db/dataloader/core/UnitTestUtils.java Introduced helper methods to generate partial output data and projected column lists.
data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataexport/producer/JsonProducerTask.java Updated column projection check and added null-handling logic using NullNode.
data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataexport/producer/JsonLineProducerTask.java Made similar projection and null-handling updates as in JsonProducerTask.
data-loader/core/src/main/java/com/scalar/db/dataloader/core/dataexport/producer/CsvProducerTask.java Updated the projection check to ensure all columns are exported when no projection is specified.
Comments suppressed due to low confidence (1)

data-loader/core/src/test/java/com/scalar/db/dataloader/core/dataexport/producer/CsvProducerTaskTest.java:64

  • The test method name references 'JsonLineString' even though it belongs to a CSV producer test. Consider renaming it to '...ShouldReturnValidCsvString' for clarity.
@Test void process_withValidResultList_withPartialProjections_shouldReturnValidJsonLineString() {

Copy link
Collaborator

@brfrn169 brfrn169 left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

Copy link
Contributor

@ypeckstadt ypeckstadt left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you.

Copy link
Contributor

@komamitsu komamitsu left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

Copy link
Contributor

@feeblefakie feeblefakie left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants