Skip to content

Conversation

@YiChengLee03
Copy link

@YiChengLee03 YiChengLee03 commented Jul 25, 2025

Description

Register Reader and Writer

Motivation and Context

Add support for Textfile reader and writer

Impact

Test Plan

Contributor checklist

  • Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== NO RELEASE NOTE ==

@YiChengLee03 YiChengLee03 marked this pull request as ready for review July 25, 2025 22:31
@YiChengLee03 YiChengLee03 requested a review from a team as a code owner July 25, 2025 22:31
@xin-zhang2 xin-zhang2 mentioned this pull request Jul 29, 2025
6 tasks
@aditi-pandit
Copy link
Contributor

@YiChengLee03 : Thanks for these code changes.

Please can you add some e2e tests for this PR.

If Text is a valid Hive storage format, then it would be good to add this as a storage format in https://github.com/prestodb/presto/blob/master/presto-native-execution/src/test/java/com/facebook/presto/nativeworker/PrestoNativeQueryRunnerUtils.java#L92, so that we can easily flip Native tests to use text format (just like we can switch between DWRF and Parquet).

@amitkdutta @xin-zhang2

zacw7
zacw7 previously approved these changes Jul 29, 2025
@YiChengLee03
Copy link
Author

@YiChengLee03 : Thanks for these code changes.

Please can you add some e2e tests for this PR.

If Text is a valid Hive storage format, then it would be good to add this as a storage format in https://github.com/prestodb/presto/blob/master/presto-native-execution/src/test/java/com/facebook/presto/nativeworker/PrestoNativeQueryRunnerUtils.java#L92, so that we can easily flip Native tests to use text format (just like we can switch between DWRF and Parquet).

@amitkdutta @xin-zhang2

Sure! Thanks for catching that!

@YiChengLee03 YiChengLee03 marked this pull request as draft July 29, 2025 21:37
@YiChengLee03 YiChengLee03 force-pushed the master branch 5 times, most recently from b7963b7 to b168d30 Compare July 30, 2025 16:59
@YiChengLee03 YiChengLee03 requested a review from zacw7 July 30, 2025 17:01
@YiChengLee03 YiChengLee03 marked this pull request as ready for review July 30, 2025 17:01
@YiChengLee03 YiChengLee03 marked this pull request as draft July 30, 2025 17:54
@YiChengLee03
Copy link
Author

Build failure tracked here and fixed here

@YiChengLee03 YiChengLee03 marked this pull request as ready for review August 1, 2025 17:41
@libianoss
Copy link

@YiChengLee03 @zacw7 can we get this PR approved and merge?Thanks.

@xin-zhang2
Copy link
Contributor

xin-zhang2 commented Sep 2, 2025

@YiChengLee03 @zacw7 Are you still working on this PR? If not, I can help with it.

@zacw7
Copy link
Member

zacw7 commented Sep 2, 2025

@YiChengLee03 @zacw7 Are you still working on this PR? If not, I can help with it.

Please feel free to take it over. I can also resubmit this change.

@xin-zhang2
Copy link
Contributor

@zacw7 Thanks! TextWriter has been added in #25673, and I've submmitted a new PR #25995 for the TextReader.

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.

5 participants