Fix writing to local filesystem - #301

Merged
Fokko merged 5 commits into
apache:mainfrom
kevinjqliu:kevinjqliu/fix-write-local-fs
Jan 29, 2024
Merged

Fix writing to local filesystem#301
Fokko merged 5 commits into
apache:mainfrom
kevinjqliu:kevinjqliu/fix-write-local-fs

Conversation

@kevinjqliu

@kevinjqliukevinjqliu commented Jan 24, 2024

Copy link
Copy Markdown
Contributor

Issue #299

This PR changes the behavior of both PyArrow and FsSpec file systems. When writing to the local file system, parent directories will be created first before writing to the file. Previously, a FileNotFoundError error is thrown when the parent directories are missing.

Testing

Added tests to tests/io/test_fsspec.py and tests/io/test_pyarrow.py
Added test_append_table test to write to the local filesystem, to illustrate the issue. It fails without the changes in this PR.
Also added the catalog_sqlite_fsspec catalog which tests writing to the local filesystem using FSSPEC_FILE_IO

@FokkoFokko left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see the need for this, but I'm reluctant to support this. You could pre-create the directories outside of PyIceberg if possible.

Comment threadpyiceberg/io/pyarrow.py Outdated
if not overwrite and self.exists() is True:
raise FileExistsError(f"Cannot create file, already exists: {self.location}")
# Parent directories must be created first in certain file systems, such as the LocalFileSystem.
self._filesystem.create_dir(os.path.dirname(self._path), recursive=True)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is typically something that we try to avoid. Iceberg is designed to work with object stores, and those don't have a notion of directories. One recommendation is even to disallow moves and listing of directories. One thing is also creating a directory. I'm not sure what the behavior is for the Arrow S3 implementation. Since some of the implementations still make a call(s):

  • For example, a list operation to check if the directory is there
  • For example, they touch a small file under the prefix to indicate that the path should be created.

Another concern is that we currently do this in Arrow, we also would need to do this for other implementations to avoid discrepancies. The concept of the FileIO is that you easily can swap them out.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@Fokko thanks for the review.

I agree with the above. The Arrow FileIO implementation might not be the best place to implement this behavior. So far both of the supported FS implementations (ARROW_FILE_IO and FSSPEC_FILE_IO) are failing to write to the local file system.

I want to make writes work for the local file system.

Looking at the Java side, there is a LocalOutputFile implementation which implements the behavior for creating parent directories.

Maybe we can implement a new FileIO implementation and make that the preferred implementation for the file:// scheme.

"file": [ARROW_FILE_IO],

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hey @kevinjqliu That sounds like a reasonable alternative to me 👍

@FokkoFokkoJan 25, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thinking a bit more about it, maybe we should still use the ArrowFileSystem in this new FileIO to make sure that we can read the table into an Arrow pa.Table.

@kevinjqliukevinjqliuJan 26, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@Fokko I updated the PR for both PyArrow and FsSpec implementations

@kevinjqliu
kevinjqliuforce-pushed the kevinjqliu/fix-write-local-fs branch from 752b63f to 1c480a5CompareJanuary 26, 2024 01:20
@kevinjqliu
kevinjqliuforce-pushed the kevinjqliu/fix-write-local-fs branch from 5ea2ceb to 5c77796CompareJanuary 26, 2024 02:12
@kevinjqliu
kevinjqliuforce-pushed the kevinjqliu/fix-write-local-fs branch from 5c77796 to 57d68f2CompareJanuary 26, 2024 02:23


SCHEME_TO_FS = {
"": _file,

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

pyarrow defaults scheme to file when no scheme is present.

return"file", uri.netloc, os.path.abspath(location)

we essentially do the same here

@kevinjqliu
kevinjqliu requested a review from FokkoJanuary 26, 2024 02:35

@FokkoFokko left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @kevinjqliu for working on this, elegant solution! LGTM

@HonahXHonahX left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM! Thanks @kevinjqliu

@Fokko
Fokko merged commit f66e365 into apache:mainJan 29, 2024
@FokkoFokko added this to the PyIceberg 0.6.0 release milestone Jan 29, 2024
@kevinjqliu
kevinjqliu deleted the kevinjqliu/fix-write-local-fs branch January 30, 2024 02:34
Sign up for freeto 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.

3 participants

@kevinjqliu@Fokko@HonahX
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Fix writing to local filesystem - #301

Merged
Fokko merged 5 commits into
apache:mainfrom
kevinjqliu:kevinjqliu/fix-write-local-fs
Jan 29, 2024
Merged

Fix writing to local filesystem#301
Fokko merged 5 commits into
apache:mainfrom
kevinjqliu:kevinjqliu/fix-write-local-fs

Conversation

@kevinjqliu

@kevinjqliukevinjqliu commented Jan 24, 2024

Copy link
Copy Markdown
Contributor

Issue #299

This PR changes the behavior of both PyArrow and FsSpec file systems. When writing to the local file system, parent directories will be created first before writing to the file. Previously, a FileNotFoundError error is thrown when the parent directories are missing.

Testing

Added tests to tests/io/test_fsspec.py and tests/io/test_pyarrow.py
Added test_append_table test to write to the local filesystem, to illustrate the issue. It fails without the changes in this PR.
Also added the catalog_sqlite_fsspec catalog which tests writing to the local filesystem using FSSPEC_FILE_IO

@FokkoFokko left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see the need for this, but I'm reluctant to support this. You could pre-create the directories outside of PyIceberg if possible.

Comment threadpyiceberg/io/pyarrow.py Outdated
if not overwrite and self.exists() is True:
raise FileExistsError(f"Cannot create file, already exists: {self.location}")
# Parent directories must be created first in certain file systems, such as the LocalFileSystem.
self._filesystem.create_dir(os.path.dirname(self._path), recursive=True)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is typically something that we try to avoid. Iceberg is designed to work with object stores, and those don't have a notion of directories. One recommendation is even to disallow moves and listing of directories. One thing is also creating a directory. I'm not sure what the behavior is for the Arrow S3 implementation. Since some of the implementations still make a call(s):

  • For example, a list operation to check if the directory is there
  • For example, they touch a small file under the prefix to indicate that the path should be created.

Another concern is that we currently do this in Arrow, we also would need to do this for other implementations to avoid discrepancies. The concept of the FileIO is that you easily can swap them out.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@Fokko thanks for the review.

I agree with the above. The Arrow FileIO implementation might not be the best place to implement this behavior. So far both of the supported FS implementations (ARROW_FILE_IO and FSSPEC_FILE_IO) are failing to write to the local file system.

I want to make writes work for the local file system.

Looking at the Java side, there is a LocalOutputFile implementation which implements the behavior for creating parent directories.

Maybe we can implement a new FileIO implementation and make that the preferred implementation for the file:// scheme.

"file": [ARROW_FILE_IO],

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hey @kevinjqliu That sounds like a reasonable alternative to me 👍

@FokkoFokkoJan 25, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thinking a bit more about it, maybe we should still use the ArrowFileSystem in this new FileIO to make sure that we can read the table into an Arrow pa.Table.

@kevinjqliukevinjqliuJan 26, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@Fokko I updated the PR for both PyArrow and FsSpec implementations

@kevinjqliu
kevinjqliuforce-pushed the kevinjqliu/fix-write-local-fs branch from 752b63f to 1c480a5CompareJanuary 26, 2024 01:20
@kevinjqliu
kevinjqliuforce-pushed the kevinjqliu/fix-write-local-fs branch from 5ea2ceb to 5c77796CompareJanuary 26, 2024 02:12
@kevinjqliu
kevinjqliuforce-pushed the kevinjqliu/fix-write-local-fs branch from 5c77796 to 57d68f2CompareJanuary 26, 2024 02:23


SCHEME_TO_FS = {
"": _file,

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

pyarrow defaults scheme to file when no scheme is present.

return"file", uri.netloc, os.path.abspath(location)

we essentially do the same here

@kevinjqliu
kevinjqliu requested a review from FokkoJanuary 26, 2024 02:35

@FokkoFokko left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @kevinjqliu for working on this, elegant solution! LGTM

@HonahXHonahX left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM! Thanks @kevinjqliu

@Fokko
Fokko merged commit f66e365 into apache:mainJan 29, 2024
@FokkoFokko added this to the PyIceberg 0.6.0 release milestone Jan 29, 2024
@kevinjqliu
kevinjqliu deleted the kevinjqliu/fix-write-local-fs branch January 30, 2024 02:34
Sign up for freeto 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.

3 participants

@kevinjqliu@Fokko@HonahX
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix writing to local filesystem - #301

Merged
Fokko merged 5 commits into
apache:mainfrom
kevinjqliu:kevinjqliu/fix-write-local-fs
Jan 29, 2024
Merged

Fix writing to local filesystem#301
Fokko merged 5 commits into
apache:mainfrom
kevinjqliu:kevinjqliu/fix-write-local-fs

Conversation

@kevinjqliu

@kevinjqliukevinjqliu commented Jan 24, 2024

Copy link
Copy Markdown
Contributor

Issue #299

This PR changes the behavior of both PyArrow and FsSpec file systems. When writing to the local file system, parent directories will be created first before writing to the file. Previously, a FileNotFoundError error is thrown when the parent directories are missing.

Testing

Added tests to tests/io/test_fsspec.py and tests/io/test_pyarrow.py
Added test_append_table test to write to the local filesystem, to illustrate the issue. It fails without the changes in this PR.
Also added the catalog_sqlite_fsspec catalog which tests writing to the local filesystem using FSSPEC_FILE_IO

@FokkoFokko left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see the need for this, but I'm reluctant to support this. You could pre-create the directories outside of PyIceberg if possible.

Comment threadpyiceberg/io/pyarrow.py Outdated
if not overwrite and self.exists() is True:
raise FileExistsError(f"Cannot create file, already exists: {self.location}")
# Parent directories must be created first in certain file systems, such as the LocalFileSystem.
self._filesystem.create_dir(os.path.dirname(self._path), recursive=True)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is typically something that we try to avoid. Iceberg is designed to work with object stores, and those don't have a notion of directories. One recommendation is even to disallow moves and listing of directories. One thing is also creating a directory. I'm not sure what the behavior is for the Arrow S3 implementation. Since some of the implementations still make a call(s):

  • For example, a list operation to check if the directory is there
  • For example, they touch a small file under the prefix to indicate that the path should be created.

Another concern is that we currently do this in Arrow, we also would need to do this for other implementations to avoid discrepancies. The concept of the FileIO is that you easily can swap them out.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@Fokko thanks for the review.

I agree with the above. The Arrow FileIO implementation might not be the best place to implement this behavior. So far both of the supported FS implementations (ARROW_FILE_IO and FSSPEC_FILE_IO) are failing to write to the local file system.

I want to make writes work for the local file system.

Looking at the Java side, there is a LocalOutputFile implementation which implements the behavior for creating parent directories.

Maybe we can implement a new FileIO implementation and make that the preferred implementation for the file:// scheme.

"file": [ARROW_FILE_IO],

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hey @kevinjqliu That sounds like a reasonable alternative to me 👍

@FokkoFokkoJan 25, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thinking a bit more about it, maybe we should still use the ArrowFileSystem in this new FileIO to make sure that we can read the table into an Arrow pa.Table.

@kevinjqliukevinjqliuJan 26, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@Fokko I updated the PR for both PyArrow and FsSpec implementations

@kevinjqliu
kevinjqliuforce-pushed the kevinjqliu/fix-write-local-fs branch from 752b63f to 1c480a5CompareJanuary 26, 2024 01:20
@kevinjqliu
kevinjqliuforce-pushed the kevinjqliu/fix-write-local-fs branch from 5ea2ceb to 5c77796CompareJanuary 26, 2024 02:12
@kevinjqliu
kevinjqliuforce-pushed the kevinjqliu/fix-write-local-fs branch from 5c77796 to 57d68f2CompareJanuary 26, 2024 02:23


SCHEME_TO_FS = {
"": _file,

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

pyarrow defaults scheme to file when no scheme is present.

return"file", uri.netloc, os.path.abspath(location)

we essentially do the same here

@kevinjqliu
kevinjqliu requested a review from FokkoJanuary 26, 2024 02:35

@FokkoFokko left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @kevinjqliu for working on this, elegant solution! LGTM

@HonahXHonahX left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM! Thanks @kevinjqliu

@Fokko
Fokko merged commit f66e365 into apache:mainJan 29, 2024
@FokkoFokko added this to the PyIceberg 0.6.0 release milestone Jan 29, 2024
@kevinjqliu
kevinjqliu deleted the kevinjqliu/fix-write-local-fs branch January 30, 2024 02:34
Sign up for freeto 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.

3 participants

@kevinjqliu@Fokko@HonahX
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix writing to local filesystem - #301

Merged
Fokko merged 5 commits into
apache:mainfrom
kevinjqliu:kevinjqliu/fix-write-local-fs
Jan 29, 2024
Merged

Fix writing to local filesystem#301
Fokko merged 5 commits into
apache:mainfrom
kevinjqliu:kevinjqliu/fix-write-local-fs

Conversation

@kevinjqliu

@kevinjqliukevinjqliu commented Jan 24, 2024

Copy link
Copy Markdown
Contributor

Issue #299

This PR changes the behavior of both PyArrow and FsSpec file systems. When writing to the local file system, parent directories will be created first before writing to the file. Previously, a FileNotFoundError error is thrown when the parent directories are missing.

Testing

Added tests to tests/io/test_fsspec.py and tests/io/test_pyarrow.py
Added test_append_table test to write to the local filesystem, to illustrate the issue. It fails without the changes in this PR.
Also added the catalog_sqlite_fsspec catalog which tests writing to the local filesystem using FSSPEC_FILE_IO

@FokkoFokko left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see the need for this, but I'm reluctant to support this. You could pre-create the directories outside of PyIceberg if possible.

Comment threadpyiceberg/io/pyarrow.py Outdated
if not overwrite and self.exists() is True:
raise FileExistsError(f"Cannot create file, already exists: {self.location}")
# Parent directories must be created first in certain file systems, such as the LocalFileSystem.
self._filesystem.create_dir(os.path.dirname(self._path), recursive=True)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is typically something that we try to avoid. Iceberg is designed to work with object stores, and those don't have a notion of directories. One recommendation is even to disallow moves and listing of directories. One thing is also creating a directory. I'm not sure what the behavior is for the Arrow S3 implementation. Since some of the implementations still make a call(s):

  • For example, a list operation to check if the directory is there
  • For example, they touch a small file under the prefix to indicate that the path should be created.

Another concern is that we currently do this in Arrow, we also would need to do this for other implementations to avoid discrepancies. The concept of the FileIO is that you easily can swap them out.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@Fokko thanks for the review.

I agree with the above. The Arrow FileIO implementation might not be the best place to implement this behavior. So far both of the supported FS implementations (ARROW_FILE_IO and FSSPEC_FILE_IO) are failing to write to the local file system.

I want to make writes work for the local file system.

Looking at the Java side, there is a LocalOutputFile implementation which implements the behavior for creating parent directories.

Maybe we can implement a new FileIO implementation and make that the preferred implementation for the file:// scheme.

"file": [ARROW_FILE_IO],

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hey @kevinjqliu That sounds like a reasonable alternative to me 👍

@FokkoFokkoJan 25, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thinking a bit more about it, maybe we should still use the ArrowFileSystem in this new FileIO to make sure that we can read the table into an Arrow pa.Table.

@kevinjqliukevinjqliuJan 26, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@Fokko I updated the PR for both PyArrow and FsSpec implementations

@kevinjqliu
kevinjqliuforce-pushed the kevinjqliu/fix-write-local-fs branch from 752b63f to 1c480a5CompareJanuary 26, 2024 01:20
@kevinjqliu
kevinjqliuforce-pushed the kevinjqliu/fix-write-local-fs branch from 5ea2ceb to 5c77796CompareJanuary 26, 2024 02:12
@kevinjqliu
kevinjqliuforce-pushed the kevinjqliu/fix-write-local-fs branch from 5c77796 to 57d68f2CompareJanuary 26, 2024 02:23


SCHEME_TO_FS = {
"": _file,

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

pyarrow defaults scheme to file when no scheme is present.

return"file", uri.netloc, os.path.abspath(location)

we essentially do the same here

@kevinjqliu
kevinjqliu requested a review from FokkoJanuary 26, 2024 02:35

@FokkoFokko left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @kevinjqliu for working on this, elegant solution! LGTM

@HonahXHonahX left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM! Thanks @kevinjqliu

@Fokko
Fokko merged commit f66e365 into apache:mainJan 29, 2024
@FokkoFokko added this to the PyIceberg 0.6.0 release milestone Jan 29, 2024
@kevinjqliu
kevinjqliu deleted the kevinjqliu/fix-write-local-fs branch January 30, 2024 02:34
Sign up for freeto 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.

3 participants

@kevinjqliu@Fokko@HonahX
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Fix writing to local filesystem - #301

Merged
Fokko merged 5 commits into
apache:mainfrom
kevinjqliu:kevinjqliu/fix-write-local-fs
Jan 29, 2024
Merged

Fix writing to local filesystem#301
Fokko merged 5 commits into
apache:mainfrom
kevinjqliu:kevinjqliu/fix-write-local-fs

Conversation

@kevinjqliu

@kevinjqliukevinjqliu commented Jan 24, 2024

Copy link
Copy Markdown
Contributor

Issue #299

This PR changes the behavior of both PyArrow and FsSpec file systems. When writing to the local file system, parent directories will be created first before writing to the file. Previously, a FileNotFoundError error is thrown when the parent directories are missing.

Testing

Added tests to tests/io/test_fsspec.py and tests/io/test_pyarrow.py
Added test_append_table test to write to the local filesystem, to illustrate the issue. It fails without the changes in this PR.
Also added the catalog_sqlite_fsspec catalog which tests writing to the local filesystem using FSSPEC_FILE_IO

@FokkoFokko left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see the need for this, but I'm reluctant to support this. You could pre-create the directories outside of PyIceberg if possible.

Comment threadpyiceberg/io/pyarrow.py Outdated
if not overwrite and self.exists() is True:
raise FileExistsError(f"Cannot create file, already exists: {self.location}")
# Parent directories must be created first in certain file systems, such as the LocalFileSystem.
self._filesystem.create_dir(os.path.dirname(self._path), recursive=True)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is typically something that we try to avoid. Iceberg is designed to work with object stores, and those don't have a notion of directories. One recommendation is even to disallow moves and listing of directories. One thing is also creating a directory. I'm not sure what the behavior is for the Arrow S3 implementation. Since some of the implementations still make a call(s):

  • For example, a list operation to check if the directory is there
  • For example, they touch a small file under the prefix to indicate that the path should be created.

Another concern is that we currently do this in Arrow, we also would need to do this for other implementations to avoid discrepancies. The concept of the FileIO is that you easily can swap them out.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@Fokko thanks for the review.

I agree with the above. The Arrow FileIO implementation might not be the best place to implement this behavior. So far both of the supported FS implementations (ARROW_FILE_IO and FSSPEC_FILE_IO) are failing to write to the local file system.

I want to make writes work for the local file system.

Looking at the Java side, there is a LocalOutputFile implementation which implements the behavior for creating parent directories.

Maybe we can implement a new FileIO implementation and make that the preferred implementation for the file:// scheme.

"file": [ARROW_FILE_IO],

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hey @kevinjqliu That sounds like a reasonable alternative to me 👍

@FokkoFokkoJan 25, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thinking a bit more about it, maybe we should still use the ArrowFileSystem in this new FileIO to make sure that we can read the table into an Arrow pa.Table.

@kevinjqliukevinjqliuJan 26, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@Fokko I updated the PR for both PyArrow and FsSpec implementations

@kevinjqliu
kevinjqliuforce-pushed the kevinjqliu/fix-write-local-fs branch from 752b63f to 1c480a5CompareJanuary 26, 2024 01:20
@kevinjqliu
kevinjqliuforce-pushed the kevinjqliu/fix-write-local-fs branch from 5ea2ceb to 5c77796CompareJanuary 26, 2024 02:12
@kevinjqliu
kevinjqliuforce-pushed the kevinjqliu/fix-write-local-fs branch from 5c77796 to 57d68f2CompareJanuary 26, 2024 02:23


SCHEME_TO_FS = {
"": _file,

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

pyarrow defaults scheme to file when no scheme is present.

return"file", uri.netloc, os.path.abspath(location)

we essentially do the same here

@kevinjqliu
kevinjqliu requested a review from FokkoJanuary 26, 2024 02:35

@FokkoFokko left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @kevinjqliu for working on this, elegant solution! LGTM

@HonahXHonahX left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM! Thanks @kevinjqliu

@Fokko
Fokko merged commit f66e365 into apache:mainJan 29, 2024
@FokkoFokko added this to the PyIceberg 0.6.0 release milestone Jan 29, 2024
@kevinjqliu
kevinjqliu deleted the kevinjqliu/fix-write-local-fs branch January 30, 2024 02:34
Sign up for freeto 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.

3 participants

@kevinjqliu@Fokko@HonahX
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix writing to local filesystem - #301

Merged
Fokko merged 5 commits into
apache:mainfrom
kevinjqliu:kevinjqliu/fix-write-local-fs
Jan 29, 2024
Merged

Fix writing to local filesystem#301
Fokko merged 5 commits into
apache:mainfrom
kevinjqliu:kevinjqliu/fix-write-local-fs

Conversation

@kevinjqliu

@kevinjqliukevinjqliu commented Jan 24, 2024

Copy link
Copy Markdown
Contributor

Issue #299

This PR changes the behavior of both PyArrow and FsSpec file systems. When writing to the local file system, parent directories will be created first before writing to the file. Previously, a FileNotFoundError error is thrown when the parent directories are missing.

Testing

Added tests to tests/io/test_fsspec.py and tests/io/test_pyarrow.py
Added test_append_table test to write to the local filesystem, to illustrate the issue. It fails without the changes in this PR.
Also added the catalog_sqlite_fsspec catalog which tests writing to the local filesystem using FSSPEC_FILE_IO

@FokkoFokko left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see the need for this, but I'm reluctant to support this. You could pre-create the directories outside of PyIceberg if possible.

Comment threadpyiceberg/io/pyarrow.py Outdated
if not overwrite and self.exists() is True:
raise FileExistsError(f"Cannot create file, already exists: {self.location}")
# Parent directories must be created first in certain file systems, such as the LocalFileSystem.
self._filesystem.create_dir(os.path.dirname(self._path), recursive=True)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is typically something that we try to avoid. Iceberg is designed to work with object stores, and those don't have a notion of directories. One recommendation is even to disallow moves and listing of directories. One thing is also creating a directory. I'm not sure what the behavior is for the Arrow S3 implementation. Since some of the implementations still make a call(s):

  • For example, a list operation to check if the directory is there
  • For example, they touch a small file under the prefix to indicate that the path should be created.

Another concern is that we currently do this in Arrow, we also would need to do this for other implementations to avoid discrepancies. The concept of the FileIO is that you easily can swap them out.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@Fokko thanks for the review.

I agree with the above. The Arrow FileIO implementation might not be the best place to implement this behavior. So far both of the supported FS implementations (ARROW_FILE_IO and FSSPEC_FILE_IO) are failing to write to the local file system.

I want to make writes work for the local file system.

Looking at the Java side, there is a LocalOutputFile implementation which implements the behavior for creating parent directories.

Maybe we can implement a new FileIO implementation and make that the preferred implementation for the file:// scheme.

"file": [ARROW_FILE_IO],

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hey @kevinjqliu That sounds like a reasonable alternative to me 👍

@FokkoFokkoJan 25, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thinking a bit more about it, maybe we should still use the ArrowFileSystem in this new FileIO to make sure that we can read the table into an Arrow pa.Table.

@kevinjqliukevinjqliuJan 26, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@Fokko I updated the PR for both PyArrow and FsSpec implementations

@kevinjqliu
kevinjqliuforce-pushed the kevinjqliu/fix-write-local-fs branch from 752b63f to 1c480a5CompareJanuary 26, 2024 01:20
@kevinjqliu
kevinjqliuforce-pushed the kevinjqliu/fix-write-local-fs branch from 5ea2ceb to 5c77796CompareJanuary 26, 2024 02:12
@kevinjqliu
kevinjqliuforce-pushed the kevinjqliu/fix-write-local-fs branch from 5c77796 to 57d68f2CompareJanuary 26, 2024 02:23


SCHEME_TO_FS = {
"": _file,

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

pyarrow defaults scheme to file when no scheme is present.

return"file", uri.netloc, os.path.abspath(location)

we essentially do the same here

@kevinjqliu
kevinjqliu requested a review from FokkoJanuary 26, 2024 02:35

@FokkoFokko left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @kevinjqliu for working on this, elegant solution! LGTM

@HonahXHonahX left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM! Thanks @kevinjqliu

@Fokko
Fokko merged commit f66e365 into apache:mainJan 29, 2024
@FokkoFokko added this to the PyIceberg 0.6.0 release milestone Jan 29, 2024
@kevinjqliu
kevinjqliu deleted the kevinjqliu/fix-write-local-fs branch January 30, 2024 02:34
Sign up for freeto 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.

3 participants

@kevinjqliu@Fokko@HonahX
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix writing to local filesystem - #301

Merged
Fokko merged 5 commits into
apache:mainfrom
kevinjqliu:kevinjqliu/fix-write-local-fs
Jan 29, 2024
Merged

Fix writing to local filesystem#301
Fokko merged 5 commits into
apache:mainfrom
kevinjqliu:kevinjqliu/fix-write-local-fs

Conversation

@kevinjqliu

@kevinjqliukevinjqliu commented Jan 24, 2024

Copy link
Copy Markdown
Contributor

Issue #299

This PR changes the behavior of both PyArrow and FsSpec file systems. When writing to the local file system, parent directories will be created first before writing to the file. Previously, a FileNotFoundError error is thrown when the parent directories are missing.

Testing

Added tests to tests/io/test_fsspec.py and tests/io/test_pyarrow.py
Added test_append_table test to write to the local filesystem, to illustrate the issue. It fails without the changes in this PR.
Also added the catalog_sqlite_fsspec catalog which tests writing to the local filesystem using FSSPEC_FILE_IO

@FokkoFokko left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see the need for this, but I'm reluctant to support this. You could pre-create the directories outside of PyIceberg if possible.

Comment threadpyiceberg/io/pyarrow.py Outdated
if not overwrite and self.exists() is True:
raise FileExistsError(f"Cannot create file, already exists: {self.location}")
# Parent directories must be created first in certain file systems, such as the LocalFileSystem.
self._filesystem.create_dir(os.path.dirname(self._path), recursive=True)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is typically something that we try to avoid. Iceberg is designed to work with object stores, and those don't have a notion of directories. One recommendation is even to disallow moves and listing of directories. One thing is also creating a directory. I'm not sure what the behavior is for the Arrow S3 implementation. Since some of the implementations still make a call(s):

  • For example, a list operation to check if the directory is there
  • For example, they touch a small file under the prefix to indicate that the path should be created.

Another concern is that we currently do this in Arrow, we also would need to do this for other implementations to avoid discrepancies. The concept of the FileIO is that you easily can swap them out.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@Fokko thanks for the review.

I agree with the above. The Arrow FileIO implementation might not be the best place to implement this behavior. So far both of the supported FS implementations (ARROW_FILE_IO and FSSPEC_FILE_IO) are failing to write to the local file system.

I want to make writes work for the local file system.

Looking at the Java side, there is a LocalOutputFile implementation which implements the behavior for creating parent directories.

Maybe we can implement a new FileIO implementation and make that the preferred implementation for the file:// scheme.

"file": [ARROW_FILE_IO],

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hey @kevinjqliu That sounds like a reasonable alternative to me 👍

@FokkoFokkoJan 25, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thinking a bit more about it, maybe we should still use the ArrowFileSystem in this new FileIO to make sure that we can read the table into an Arrow pa.Table.

@kevinjqliukevinjqliuJan 26, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@Fokko I updated the PR for both PyArrow and FsSpec implementations

@kevinjqliu
kevinjqliuforce-pushed the kevinjqliu/fix-write-local-fs branch from 752b63f to 1c480a5CompareJanuary 26, 2024 01:20
@kevinjqliu
kevinjqliuforce-pushed the kevinjqliu/fix-write-local-fs branch from 5ea2ceb to 5c77796CompareJanuary 26, 2024 02:12
@kevinjqliu
kevinjqliuforce-pushed the kevinjqliu/fix-write-local-fs branch from 5c77796 to 57d68f2CompareJanuary 26, 2024 02:23


SCHEME_TO_FS = {
"": _file,

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

pyarrow defaults scheme to file when no scheme is present.

return"file", uri.netloc, os.path.abspath(location)

we essentially do the same here

@kevinjqliu
kevinjqliu requested a review from FokkoJanuary 26, 2024 02:35

@FokkoFokko left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @kevinjqliu for working on this, elegant solution! LGTM

@HonahXHonahX left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM! Thanks @kevinjqliu

@Fokko
Fokko merged commit f66e365 into apache:mainJan 29, 2024
@FokkoFokko added this to the PyIceberg 0.6.0 release milestone Jan 29, 2024
@kevinjqliu
kevinjqliu deleted the kevinjqliu/fix-write-local-fs branch January 30, 2024 02:34
Sign up for freeto 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.

3 participants

@kevinjqliu@Fokko@HonahX
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Fix writing to local filesystem - #301

Merged
Fokko merged 5 commits into
apache:mainfrom
kevinjqliu:kevinjqliu/fix-write-local-fs
Jan 29, 2024
Merged

Fix writing to local filesystem#301
Fokko merged 5 commits into
apache:mainfrom
kevinjqliu:kevinjqliu/fix-write-local-fs

Conversation

@kevinjqliu

@kevinjqliukevinjqliu commented Jan 24, 2024

Copy link
Copy Markdown
Contributor

Issue #299

This PR changes the behavior of both PyArrow and FsSpec file systems. When writing to the local file system, parent directories will be created first before writing to the file. Previously, a FileNotFoundError error is thrown when the parent directories are missing.

Testing

Added tests to tests/io/test_fsspec.py and tests/io/test_pyarrow.py
Added test_append_table test to write to the local filesystem, to illustrate the issue. It fails without the changes in this PR.
Also added the catalog_sqlite_fsspec catalog which tests writing to the local filesystem using FSSPEC_FILE_IO

@FokkoFokko left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see the need for this, but I'm reluctant to support this. You could pre-create the directories outside of PyIceberg if possible.

Comment threadpyiceberg/io/pyarrow.py Outdated
if not overwrite and self.exists() is True:
raise FileExistsError(f"Cannot create file, already exists: {self.location}")
# Parent directories must be created first in certain file systems, such as the LocalFileSystem.
self._filesystem.create_dir(os.path.dirname(self._path), recursive=True)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is typically something that we try to avoid. Iceberg is designed to work with object stores, and those don't have a notion of directories. One recommendation is even to disallow moves and listing of directories. One thing is also creating a directory. I'm not sure what the behavior is for the Arrow S3 implementation. Since some of the implementations still make a call(s):

  • For example, a list operation to check if the directory is there
  • For example, they touch a small file under the prefix to indicate that the path should be created.

Another concern is that we currently do this in Arrow, we also would need to do this for other implementations to avoid discrepancies. The concept of the FileIO is that you easily can swap them out.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@Fokko thanks for the review.

I agree with the above. The Arrow FileIO implementation might not be the best place to implement this behavior. So far both of the supported FS implementations (ARROW_FILE_IO and FSSPEC_FILE_IO) are failing to write to the local file system.

I want to make writes work for the local file system.

Looking at the Java side, there is a LocalOutputFile implementation which implements the behavior for creating parent directories.

Maybe we can implement a new FileIO implementation and make that the preferred implementation for the file:// scheme.

"file": [ARROW_FILE_IO],

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hey @kevinjqliu That sounds like a reasonable alternative to me 👍

@FokkoFokkoJan 25, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thinking a bit more about it, maybe we should still use the ArrowFileSystem in this new FileIO to make sure that we can read the table into an Arrow pa.Table.

@kevinjqliukevinjqliuJan 26, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@Fokko I updated the PR for both PyArrow and FsSpec implementations

@kevinjqliu
kevinjqliuforce-pushed the kevinjqliu/fix-write-local-fs branch from 752b63f to 1c480a5CompareJanuary 26, 2024 01:20
@kevinjqliu
kevinjqliuforce-pushed the kevinjqliu/fix-write-local-fs branch from 5ea2ceb to 5c77796CompareJanuary 26, 2024 02:12
@kevinjqliu
kevinjqliuforce-pushed the kevinjqliu/fix-write-local-fs branch from 5c77796 to 57d68f2CompareJanuary 26, 2024 02:23


SCHEME_TO_FS = {
"": _file,

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

pyarrow defaults scheme to file when no scheme is present.

return"file", uri.netloc, os.path.abspath(location)

we essentially do the same here

@kevinjqliu
kevinjqliu requested a review from FokkoJanuary 26, 2024 02:35

@FokkoFokko left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @kevinjqliu for working on this, elegant solution! LGTM

@HonahXHonahX left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM! Thanks @kevinjqliu

@Fokko
Fokko merged commit f66e365 into apache:mainJan 29, 2024
@FokkoFokko added this to the PyIceberg 0.6.0 release milestone Jan 29, 2024
@kevinjqliu
kevinjqliu deleted the kevinjqliu/fix-write-local-fs branch January 30, 2024 02:34
Sign up for freeto 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.

3 participants

@kevinjqliu@Fokko@HonahX