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

fix: add support for microsoft onelake #1564

Closed
wants to merge 19 commits into from

Conversation

vmuddassir-msft
Copy link
Contributor

No description provided.

@github-actions github-actions bot added binding/rust Issues for the Rust crate rust labels Jul 26, 2023
@github-actions
Copy link

ACTION NEEDED

delta-rs follows the Conventional Commits specification for release automation.

The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification.

@vmuddassir-msft vmuddassir-msft changed the title Changes for one-lake fix issue #1418 Changes required to add Support for Microsoft OneLake Jul 27, 2023
@vmuddassir-msft
Copy link
Contributor Author

vmuddassir-msft commented Jul 27, 2023

Made code change to test utils to handle abfss (abfss://@onelake.dfs.fabric.microsoft.com)
Reads/Writes need to use a fully qualified https or abfss (onelake.dfs.fabric.microsoft.com/ws-id/artifact-id

Issue #1418

@vmuddassir-msft vmuddassir-msft changed the title Changes required to add Support for Microsoft OneLake Fix:Add Support for Microsoft OneLake Jul 27, 2023
@vmuddassir-msft vmuddassir-msft changed the title Fix:Add Support for Microsoft OneLake fix:Add Support for Microsoft OneLake Jul 27, 2023
@vmuddassir-msft vmuddassir-msft changed the title fix:Add Support for Microsoft OneLake fix: add support for microsoft onelake Jul 27, 2023
rust/src/test_utils.rs Outdated Show resolved Hide resolved
rust/src/test_utils.rs Outdated Show resolved Hide resolved
rust/src/test_utils.rs Outdated Show resolved Hide resolved
vmuddassir-msft and others added 5 commits August 21, 2023 11:23
Co-authored-by: Christopher Watford <132389385+watfordkcf@users.noreply.github.com>
Co-authored-by: Christopher Watford <132389385+watfordkcf@users.noreply.github.com>
Co-authored-by: Christopher Watford <132389385+watfordkcf@users.noreply.github.com>
Co-authored-by: Christopher Watford <132389385+watfordkcf@users.noreply.github.com>
@rtyler
Copy link
Member

rtyler commented Sep 6, 2023

@vmuddassir-msft are you getting to a point to where you're ready to put this into Review?

@vmuddassir-msft
Copy link
Contributor Author

vmuddassir-msft commented Sep 7, 2023

@vmuddassir-msft are you getting to a point to where you're ready to put this into Review?

Delta-rs needs to be upgraded to the latest version of object_store v0.7.0 , in order to add Support for Microsoft Fabric / OneLake . I have upgraded the versions for object_store with other dependencies which reference object_store v0.6.1

I am facing an issue after the upgrade :
image

This is because datafusion seems to be internally using the dependency crate arrow-array v45 while the latest version is arrow-array = { version = "46.0.0" }

image

@vmuddassir-msft vmuddassir-msft marked this pull request as ready for review September 7, 2023 11:03
@wjones127
Copy link
Collaborator

This is because datafusion seems to be internally using the dependency crate arrow-array v45 while the latest version is arrow-array = { version = "46.0.0" }

Yes, we need to wait for datafusion to upgrade versions of arrow before we can. Can you upgrade object store independent of arrow?

@vmuddassir-msft
Copy link
Contributor Author

vmuddassir-msft commented Sep 8, 2023

This is because datafusion seems to be internally using the dependency crate arrow-array v45 while the latest version is arrow-array = { version = "46.0.0" }

Yes, we need to wait for datafusion to upgrade versions of arrow before we can. Can you upgrade object store independent of arrow?

Unable to upgrade Object store independently as this is also a dependency for parquet and the latest version of paraquet v46 has linked dependencies to the latest versions of all arrow related crates which includes arrow-array

image

@wjones127
Copy link
Collaborator

@vmuddassir-msft understood. The next version of datafusion is out for vote, so it should be released sometime next week. Once that happens we should be able to get this PR in a merge-able state. :)

@@ -372,7 +372,8 @@ pub(crate) fn divide_by_partition_values(
})
.collect::<Result<Vec<_>, DeltaWriterError>>()?;

let partition_ranges = lexicographical_partition_ranges(sorted_partition_columns.as_slice())?;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

#[allow(warnings)] for now as :lexicographical_partition_ranges is Deprecated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wjones127 / @rtyler - Datafusion packages have been upgraded

Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI another dedicated PR was merged to upgrade datafusion, so you'll need to address conflicts with that. Should be quick.

@vmuddassir-msft
Copy link
Contributor Author

vmuddassir-msft commented Sep 15, 2023

@vmuddassir-msft are you getting to a point to where you're ready to put this into Review?

@rtyler - PR is ready to be reviewed

The test for onelake is failing as this requires a bearer token , test can be marked as ignored

image

@rtyler rtyler self-assigned this Sep 15, 2023
@rtyler rtyler added this to the Rust v0.16 milestone Sep 15, 2023
rtyler
rtyler previously approved these changes Sep 15, 2023
@rtyler rtyler enabled auto-merge (rebase) September 15, 2023 21:58
@rtyler
Copy link
Member

rtyler commented Sep 15, 2023

@vmuddassir-msft is your expectation that the test will never be run? (in which case we should just delete it), or do you want to configure a bearer token in GitHub secrets so that at least CI can run?

auto-merge was automatically disabled September 19, 2023 04:27

Head branch was pushed to by a user without write access

@rtyler
Copy link
Member

rtyler commented Sep 19, 2023

Closing this in favor of the rebased #1642

@rtyler rtyler closed this Sep 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
binding/rust Issues for the Rust crate rust
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants