-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Make sure that writeOptions is used when passed to the writeFiles function #652
Make sure that writeOptions is used when passed to the writeFiles function #652
Conversation
@mahmoudmahdi24 Thanks for making the PR. Is there any particular option that you wanted to pass in? |
Hello @liwensun |
1d879a9
to
2f084bb
Compare
@mahmoudmahdi24 If you don't have a specific option, I'm inclined to close this. This change will enable users to pass parquet options to Delta, and may cause surprising behaviors. |
Hi @zsxwing, did you see my question? |
@noslowerdna Sorry. I missed your previous comment. My concern is about blindly passing options to Parquet. If there were options that could affect the data to be written, we would break the compatibility. In addition, the |
@zsxwing Thanks for the response. I agree, it should not be blind. Let's just handle the special
The implementation of that configuration resolution process can be found here in the Similarly the
|
@mahmoudmahdi24 I would appreciate your feedback here: #1017 . . . thanks! |
This PR should be closed, #1017 addresses this and has been merged. |
Thanks! Closing this one as #1017 addressed the issue. |
I have realized while digging into the code that we have a function called
writeFiles
inside theTransactionaWriter
with an unused parameterwriteOptions
.This Pull Request changes the code so it's now possible to pass
DeltaOptions
to thewriteFiles
function and use its values during the write.Before these changes, the code never used what we pass as write options.