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

Ensure putSnapshot path honoring case insensitive contract #85

Merged
merged 11 commits into from May 1, 2024

Conversation

autumnust
Copy link
Collaborator

@autumnust autumnust commented Apr 24, 2024

Summary

This is a bug fixes for put-snapshot code path, where UUID-extraction process is using tableId and databaseId from the request itself. Those ids, if directly obtained from top-level request body, can lost casing if the requests were from platform like Spark SQL. Since the underlying storage (e.g. HDFS ) is case sensitive in its path URL, we will need to ensure the original casing info when issue the first commit as part of CTAS is preserved in the process of UUID-extraction of the second commit.

The other parts in this PR is to ensure, when put is happening, the tableDto is not always built from scratch when there's existing object discovered by findById method previously. This is done by switch orElse method to orElseGet, in which the latter will only call the supplier lazily when the calling object is absent. This leads to a wasteful implementation as well as confusion on the code stack.

This PR also include:

  • Some refactoring to share code with existing code.
  • Ensure text-fixtures module has its repository horning case insensitive contract so that this behavior can be tested through embedded instances.
  • Testing the casing contract in both SQL and Catalog API.

Changes

  • Client-facing API Changes
  • Internal API Changes
  • Bug Fixes
  • New Features
  • Performance Improvements
  • Code Style
  • Refactoring
  • Documentation
  • Tests

For all the boxes checked, please include additional details of the changes made in this pull request.

Testing Done

  • Manually Tested on local docker setup. Please include commands ran, and their output.
  • Added new tests for the changes made.
  • Updated existing tests to reflect the changes made.
  • No tests added or updated. Please explain why. If unsure, please feel free to ask for help.
  • Some other form of testing like staging or soak time in production. Please explain.

For all the boxes checked, include a detailed description of the testing done for the changes made in this pull request.

Additional Information

  • Breaking Changes
  • Deprecations
  • Large PR broken into smaller PRs, and PR plan linked in the description.

For all the boxes checked, include additional details of the changes made in this pull request.

Copy link
Collaborator

@jiang95-dev jiang95-dev left a comment

Choose a reason for hiding this comment

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

Why is the issue related to CTAS? It failed in the regular write path.

@autumnust
Copy link
Collaborator Author

Why is the issue related to CTAS? It failed in the regular write path.

The ticket you gave has the log line which located in this class.

@autumnust autumnust changed the title Ensure CTAS path honoring case insensitive contract Ensure putSnapshot path honoring case insensitive contract Apr 27, 2024
@autumnust autumnust closed this Apr 27, 2024
@autumnust autumnust reopened this Apr 27, 2024
Copy link
Collaborator

@HotSushi HotSushi left a comment

Choose a reason for hiding this comment

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

Thanks for these changes! unit tests are also strong to tackle case Sensitive issues. Few recommendations

HotSushi
HotSushi previously approved these changes Apr 30, 2024
Copy link
Collaborator

@HotSushi HotSushi left a comment

Choose a reason for hiding this comment

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

Few comments

@autumnust autumnust closed this May 1, 2024
@autumnust autumnust reopened this May 1, 2024
@autumnust autumnust closed this May 1, 2024
@autumnust autumnust reopened this May 1, 2024
Copy link
Collaborator

@rohitkum2506 rohitkum2506 left a comment

Choose a reason for hiding this comment

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

Thanks for addressing comments.

@autumnust autumnust merged commit d824024 into linkedin:main May 1, 2024
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants