GH-34386: [C++] Add a PathFromUriOrPath method - #34420

Merged
westonpace merged 8 commits into
apache:mainfrom
westonpace:feature/GH-34386--path-from-urireset
May 4, 2023
Merged

GH-34386: [C++] Add a PathFromUriOrPath method#34420
westonpace merged 8 commits into
apache:mainfrom
westonpace:feature/GH-34386--path-from-urireset

Conversation

@westonpace

@westonpacewestonpace commented Mar 3, 2023•
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Member

Rationale for this change

We have some URI parsing indirectly exposed through FilesystemFromUri. However, this isn't very useful if the user has multiple URIs or if they already have a filesystem. This method allows that same URI handling to be used even if the user already has a filesystem.

What changes are included in this PR?

Adds a new arrow::fs::PathFromUriOrPath method

Are these changes tested?

Yes, via unit tests

Are there any user-facing changes?

There is a new API method but no changes to any existing APIs

@github-actionsgithub-actionsBot added the awaiting committer review Awaiting committer review label Mar 3, 2023
@github-actions

github-actionsBot commented Mar 3, 2023

Copy link
Copy Markdown

@github-actions

github-actionsBot commented Mar 3, 2023

Copy link
Copy Markdown

⚠️ GitHub issue #34386has been automatically assigned in GitHub to PR creator.

@lidavidm

lidavidm commented Mar 7, 2023

Copy link
Copy Markdown
Member

Oh neat, thank you. I'll try to take a look at this soon

lidavidm
lidavidm reviewed Mar 10, 2023
View reviewed changes
Comment threadcpp/src/arrow/filesystem/filesystem.h Outdated
Comment threadcpp/src/arrow/filesystem/filesystem.cc Outdated
Comment threadcpp/src/arrow/filesystem/filesystem.cc Outdated
@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Mar 17, 2023
@amol-

amol- commented Mar 30, 2023

Copy link
Copy Markdown
Member

Closing because it has been untouched for a while, in case it's still relevant feel free to reopen and move it forward 👍

@amol-amol- closed this Mar 30, 2023
@lidavidm

lidavidm commented Apr 12, 2023

Copy link
Copy Markdown
Member

Not sure why this was unceremoniously closed. Weston, do you think you would still have time to look at this? It would be useful in #35034

@lidavidmlidavidm mentioned this pull request Apr 12, 2023
Merged
@westonpacewestonpace reopened this Apr 14, 2023
@westonpace

westonpace commented Apr 14, 2023

Copy link
Copy Markdown
MemberAuthor

Yes, I will try and look at this. I think it still needs some changes.

@westonpace
westonpaceforce-pushed the feature/GH-34386--path-from-urireset branch from 79dc526 to 9100358CompareMay 2, 2023 16:17
@github-actionsgithub-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels May 2, 2023
@westonpacewestonpace mentioned this pull request May 3, 2023
Closed
@westonpace

westonpace commented May 3, 2023

Copy link
Copy Markdown
MemberAuthor

I unified the various paths using a helper method in util_internal.cc. This highlighted one small difference between the various implementations. The s3 and gcs filesystems remove any trailing slash in the path while the hdfs and local filesystems do not. I have opened #35399 for any discussion on that difference but, for the moment, I have changed all the filesystems to always remove the trailing slash for consistency.

@github-actionsgithub-actionsBot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels May 3, 2023
@lidavidm

lidavidm commented May 3, 2023

Copy link
Copy Markdown
Member

Hmm, I suppose the trailing slash could change listing behavior in S3.

@westonpace

westonpace commented May 3, 2023

Copy link
Copy Markdown
MemberAuthor

Hmm, I suppose the trailing slash could change listing behavior in S3.

S3 was already removing the trailing slash so there is no change to the S3 behavior. This only affected the local filesystem.

@lidavidm

lidavidm commented May 3, 2023

Copy link
Copy Markdown
Member

Ah, I got that mixed up. LGTM then

@westonpace
westonpaceforce-pushed the feature/GH-34386--path-from-urireset branch from da56f29 to c89e9caCompareMay 4, 2023 15:42
lidavidm
lidavidm approved these changes May 4, 2023
View reviewed changes
Comment threadcpp/src/arrow/filesystem/path_util.cc
Comment on lines +137 to +142
#ifdef _WIN32
if (preserve_root && key.size() == 3 && key[1] == ':' && key[0] != '/') {
// If the user gives us C:/ then don't return C:
return key;
}
#endif

@lidavidmlidavidmMay 4, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh fun.

@westonpace

westonpace commented May 4, 2023

Copy link
Copy Markdown
MemberAuthor

There are a few errors but they seem to be unrelated. I'm going to merge this.

@westonpace
westonpace merged commit 7ca7724 into apache:mainMay 4, 2023
@ursabot

ursabot commented May 5, 2023

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = febd0ff and contender = 7ca7724. 7ca7724 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️2.27% ⬆️0.58%] test-mac-arm
[Finished ⬇️0.26% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️1.96% ⬆️0.93%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 7ca77241 ec2-t3-xlarge-us-east-2
[Finished] 7ca77241 test-mac-arm
[Finished] 7ca77241 ursa-i9-9960x
[Finished] 7ca77241 ursa-thinkcentre-m75q
[Finished] febd0ff1 ec2-t3-xlarge-us-east-2
[Finished] febd0ff1 test-mac-arm
[Finished] febd0ff1 ursa-i9-9960x
[Finished] febd0ff1 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@ursabot

ursabot commented May 5, 2023

Copy link
Copy Markdown

['Python', 'R'] benchmarks have high level of regressions.
test-mac-arm

liujiacheng777 pushed a commit to LoongArch-Python/arrow that referenced this pull request May 11, 2023
@westonpace@liujiacheng777
apacheGH-34386: [C++] Add a PathFromUriOrPath method (apache#34420)
169db77
### Rationale for this change
We have some URI parsing indirectly exposed through FilesystemFromUri. However, this isn't very useful if the user has multiple URIs or if they already have a filesystem. This method allows that same URI handling to be used even if the user already has a filesystem.
### What changes are included in this PR?
Adds a new arrow::fs::PathFromUriOrPath method
### Are these changes tested?
Yes, via unit tests
### Are there any user-facing changes?
There is a new API method but no changes to any existing APIs
* Closes: apache#34386
Authored-by: Weston Pace <weston.pace@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
ArgusLi pushed a commit to Bit-Quill/arrow that referenced this pull request May 15, 2023
@westonpace@ArgusLi
apacheGH-34386: [C++] Add a PathFromUriOrPath method (apache#34420)
0e727b9
### Rationale for this change
We have some URI parsing indirectly exposed through FilesystemFromUri. However, this isn't very useful if the user has multiple URIs or if they already have a filesystem. This method allows that same URI handling to be used even if the user already has a filesystem.
### What changes are included in this PR?
Adds a new arrow::fs::PathFromUriOrPath method
### Are these changes tested?
Yes, via unit tests
### Are there any user-facing changes?
There is a new API method but no changes to any existing APIs
* Closes: apache#34386
Authored-by: Weston Pace <weston.pace@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lidavidmlidavidmlidavidm approved these changes

Assignees

No one assigned

Labels

awaiting mergeAwaiting mergeComponent: C++

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[C++] Handle URI paths for filesystems?

4 participants

@westonpace@lidavidm@amol-@ursabot
, '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
GH-34386: [C++] Add a PathFromUriOrPath method - #34420
Merged
westonpace merged 8 commits into
apache:mainfrom 
westonpace:feature/GH-34386--path-from-urireset
May 4, 2023
Merged
GH-34386: [C++] Add a PathFromUriOrPath method#34420
westonpace merged 8 commits into
apache:mainfrom 
westonpace:feature/GH-34386--path-from-urireset
Conversation
@westonpace
@westonpacewestonpace
commented
Mar 3, 2023•

edited by github-actions
Bot
Loading

Copy link


Copy Markdown

Member
Rationale for this change
We have some URI parsing indirectly exposed through FilesystemFromUri. However, this isn't very useful if the user has multiple URIs or if they already have a filesystem. This method allows that same URI handling to be used even if the user already has a filesystem.
What changes are included in this PR?
Adds a new arrow::fs::PathFromUriOrPath method
Are these changes tested?
Yes, via unit tests
Are there any user-facing changes?
There is a new API method but no changes to any existing APIs
@github-actionsgithub-actionsBot
added
the 
awaiting committer review
Awaiting committer review
label
Mar 3, 2023
@github-actions
github-actionsBot
commented
Mar 3, 2023

Copy link


Copy Markdown

@github-actions
github-actionsBot
commented
Mar 3, 2023

Copy link


Copy Markdown

⚠️ GitHub issue #34386has been automatically assigned in GitHub to PR creator.
@lidavidm
lidavidm
commented
Mar 7, 2023

Copy link


Copy Markdown

Member
Oh neat, thank you. I'll try to take a look at this soon
lidavidm
lidavidm
reviewed
Mar 10, 2023
View reviewed changes
Comment threadcpp/src/arrow/filesystem/filesystem.h
Outdated

Comment threadcpp/src/arrow/filesystem/filesystem.cc
Outdated

Comment threadcpp/src/arrow/filesystem/filesystem.cc
Outdated

@github-actionsgithub-actionsBot
added

awaiting changes
Awaiting changes
and removed

awaiting committer review
Awaiting committer review
labels
Mar 17, 2023
@amol-
amol-
commented
Mar 30, 2023

Copy link


Copy Markdown

Member
Closing because it has been untouched for a while, in case it's still relevant feel free to reopen and move it forward 👍
@amol-amol-
closed this
Mar 30, 2023
@lidavidm
lidavidm
commented
Apr 12, 2023

Copy link


Copy Markdown

Member
Not sure why this was unceremoniously closed. Weston, do you think you would still have time to look at this? It would be useful in #35034
@lidavidmlidavidm
mentioned this pull request
Apr 12, 2023
 Merged

@westonpacewestonpace
reopened this
Apr 14, 2023
@westonpace
westonpace
commented
Apr 14, 2023

Copy link


Copy Markdown

MemberAuthor
Yes, I will try and look at this. I think it still needs some changes.
@westonpace
westonpaceforce-pushed
the
feature/GH-34386--path-from-urireset
branch
from
79dc526 to
9100358CompareMay 2, 2023 16:17
@github-actionsgithub-actionsBot
added

awaiting change review
Awaiting change review
and removed

awaiting changes
Awaiting changes
labels
May 2, 2023
@westonpacewestonpace
mentioned this pull request
May 3, 2023

Closed

@westonpace
westonpace
commented
May 3, 2023

Copy link


Copy Markdown

MemberAuthor
I unified the various paths using a helper method in util_internal.cc. This highlighted one small difference between the various implementations. The s3 and gcs filesystems remove any trailing slash in the path while the hdfs and local filesystems do not. I have opened #35399 for any discussion on that difference but, for the moment, I have changed all the filesystems to always remove the trailing slash for consistency.
@github-actionsgithub-actionsBot
added

awaiting merge
Awaiting merge
and removed

awaiting change review
Awaiting change review
labels
May 3, 2023
@lidavidm
lidavidm
commented
May 3, 2023

Copy link


Copy Markdown

Member
Hmm, I suppose the trailing slash could change listing behavior in S3.
@westonpace
westonpace
commented
May 3, 2023

Copy link


Copy Markdown

MemberAuthor
Hmm, I suppose the trailing slash could change listing behavior in S3.
S3 was already removing the trailing slash so there is no change to the S3 behavior. This only affected the local filesystem.
@lidavidm
lidavidm
commented
May 3, 2023

Copy link


Copy Markdown

Member
Ah, I got that mixed up. LGTM then
@westonpace
westonpaceforce-pushed
the
feature/GH-34386--path-from-urireset
branch
from
da56f29 to
c89e9caCompareMay 4, 2023 15:42
lidavidm
lidavidm
approved these changes
May 4, 2023
View reviewed changes
Comment threadcpp/src/arrow/filesystem/path_util.cc

Comment on lines

+137
 to 
+142

#ifdef _WIN32
 if (preserve_root && key.size() == 3 && key[1] == ':' && key[0] != '/') {
 // If the user gives us C:/ then don't return C:
 return key;
 }
#endif
@lidavidmlidavidmMay 4, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Oh fun.
@westonpace
westonpace
commented
May 4, 2023

Copy link


Copy Markdown

MemberAuthor
There are a few errors but they seem to be unrelated. I'm going to merge this.
@westonpace
westonpace
merged commit 7ca7724
into
apache:mainMay 4, 2023
@ursabot
ursabot
commented
May 5, 2023

Copy link


Copy Markdown

Benchmark runs are scheduled for baseline = febd0ff and contender = 7ca7724. 7ca7724 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.

Conbench compare runs links:

[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2

[Finished ⬇️2.27% ⬆️0.58%] test-mac-arm

[Finished ⬇️0.26% ⬆️0.0%] ursa-i9-9960x

[Finished ⬇️1.96% ⬆️0.93%] ursa-thinkcentre-m75q

Buildkite builds:

[Finished] 7ca77241 ec2-t3-xlarge-us-east-2

[Finished] 7ca77241 test-mac-arm

[Finished] 7ca77241 ursa-i9-9960x

[Finished] 7ca77241 ursa-thinkcentre-m75q

[Finished] febd0ff1 ec2-t3-xlarge-us-east-2

[Finished] febd0ff1 test-mac-arm

[Finished] febd0ff1 ursa-i9-9960x

[Finished] febd0ff1 ursa-thinkcentre-m75q

Supported benchmarks:

ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True

test-mac-arm: Supported benchmark langs: C++, Python, R

ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript

ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
@ursabot
ursabot
commented
May 5, 2023

Copy link


Copy Markdown

['Python', 'R'] benchmarks have high level of regressions.
test-mac-arm
liujiacheng777 pushed a commit
to LoongArch-Python/arrow
that referenced
this pull request
May 11, 2023
@westonpace@liujiacheng777
apacheGH-34386: [C++] Add a PathFromUriOrPath method (apache#34420)
169db77
### Rationale for this change
We have some URI parsing indirectly exposed through FilesystemFromUri. However, this isn't very useful if the user has multiple URIs or if they already have a filesystem. This method allows that same URI handling to be used even if the user already has a filesystem.
### What changes are included in this PR?
Adds a new arrow::fs::PathFromUriOrPath method
### Are these changes tested?
Yes, via unit tests
### Are there any user-facing changes?
There is a new API method but no changes to any existing APIs
* Closes: apache#34386
Authored-by: Weston Pace <weston.pace@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
ArgusLi pushed a commit
to Bit-Quill/arrow
that referenced
this pull request
May 15, 2023
@westonpace@ArgusLi
apacheGH-34386: [C++] Add a PathFromUriOrPath method (apache#34420)
0e727b9
### Rationale for this change
We have some URI parsing indirectly exposed through FilesystemFromUri. However, this isn't very useful if the user has multiple URIs or if they already have a filesystem. This method allows that same URI handling to be used even if the user already has a filesystem.
### What changes are included in this PR?
Adds a new arrow::fs::PathFromUriOrPath method
### Are these changes tested?
Yes, via unit tests
### Are there any user-facing changes?
There is a new API method but no changes to any existing APIs
* Closes: apache#34386
Authored-by: Weston Pace <weston.pace@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@lidavidmlidavidmlidavidm approved these changes

Assignees


No one assigned


Labels

awaiting mergeAwaiting mergeComponent: C++

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

[C++] Handle URI paths for filesystems?


4 participants

@westonpace@lidavidm@amol-@ursabot

, '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
GH-34386: [C++] Add a PathFromUriOrPath method - #34420
Merged
westonpace merged 8 commits into
apache:mainfrom 
westonpace:feature/GH-34386--path-from-urireset
May 4, 2023
Merged
GH-34386: [C++] Add a PathFromUriOrPath method#34420
westonpace merged 8 commits into
apache:mainfrom 
westonpace:feature/GH-34386--path-from-urireset
Conversation
@westonpace
@westonpacewestonpace
commented
Mar 3, 2023•

edited by github-actions
Bot
Loading

Copy link


Copy Markdown

Member
Rationale for this change
We have some URI parsing indirectly exposed through FilesystemFromUri. However, this isn't very useful if the user has multiple URIs or if they already have a filesystem. This method allows that same URI handling to be used even if the user already has a filesystem.
What changes are included in this PR?
Adds a new arrow::fs::PathFromUriOrPath method
Are these changes tested?
Yes, via unit tests
Are there any user-facing changes?
There is a new API method but no changes to any existing APIs
@github-actionsgithub-actionsBot
added
the 
awaiting committer review
Awaiting committer review
label
Mar 3, 2023
@github-actions
github-actionsBot
commented
Mar 3, 2023

Copy link


Copy Markdown

@github-actions
github-actionsBot
commented
Mar 3, 2023

Copy link


Copy Markdown

⚠️ GitHub issue #34386has been automatically assigned in GitHub to PR creator.
@lidavidm
lidavidm
commented
Mar 7, 2023

Copy link


Copy Markdown

Member
Oh neat, thank you. I'll try to take a look at this soon
lidavidm
lidavidm
reviewed
Mar 10, 2023
View reviewed changes
Comment threadcpp/src/arrow/filesystem/filesystem.h
Outdated

Comment threadcpp/src/arrow/filesystem/filesystem.cc
Outdated

Comment threadcpp/src/arrow/filesystem/filesystem.cc
Outdated

@github-actionsgithub-actionsBot
added

awaiting changes
Awaiting changes
and removed

awaiting committer review
Awaiting committer review
labels
Mar 17, 2023
@amol-
amol-
commented
Mar 30, 2023

Copy link


Copy Markdown

Member
Closing because it has been untouched for a while, in case it's still relevant feel free to reopen and move it forward 👍
@amol-amol-
closed this
Mar 30, 2023
@lidavidm
lidavidm
commented
Apr 12, 2023

Copy link


Copy Markdown

Member
Not sure why this was unceremoniously closed. Weston, do you think you would still have time to look at this? It would be useful in #35034
@lidavidmlidavidm
mentioned this pull request
Apr 12, 2023
 Merged

@westonpacewestonpace
reopened this
Apr 14, 2023
@westonpace
westonpace
commented
Apr 14, 2023

Copy link


Copy Markdown

MemberAuthor
Yes, I will try and look at this. I think it still needs some changes.
@westonpace
westonpaceforce-pushed
the
feature/GH-34386--path-from-urireset
branch
from
79dc526 to
9100358CompareMay 2, 2023 16:17
@github-actionsgithub-actionsBot
added

awaiting change review
Awaiting change review
and removed

awaiting changes
Awaiting changes
labels
May 2, 2023
@westonpacewestonpace
mentioned this pull request
May 3, 2023

Closed

@westonpace
westonpace
commented
May 3, 2023

Copy link


Copy Markdown

MemberAuthor
I unified the various paths using a helper method in util_internal.cc. This highlighted one small difference between the various implementations. The s3 and gcs filesystems remove any trailing slash in the path while the hdfs and local filesystems do not. I have opened #35399 for any discussion on that difference but, for the moment, I have changed all the filesystems to always remove the trailing slash for consistency.
@github-actionsgithub-actionsBot
added

awaiting merge
Awaiting merge
and removed

awaiting change review
Awaiting change review
labels
May 3, 2023
@lidavidm
lidavidm
commented
May 3, 2023

Copy link


Copy Markdown

Member
Hmm, I suppose the trailing slash could change listing behavior in S3.
@westonpace
westonpace
commented
May 3, 2023

Copy link


Copy Markdown

MemberAuthor
Hmm, I suppose the trailing slash could change listing behavior in S3.
S3 was already removing the trailing slash so there is no change to the S3 behavior. This only affected the local filesystem.
@lidavidm
lidavidm
commented
May 3, 2023

Copy link


Copy Markdown

Member
Ah, I got that mixed up. LGTM then
@westonpace
westonpaceforce-pushed
the
feature/GH-34386--path-from-urireset
branch
from
da56f29 to
c89e9caCompareMay 4, 2023 15:42
lidavidm
lidavidm
approved these changes
May 4, 2023
View reviewed changes
Comment threadcpp/src/arrow/filesystem/path_util.cc

Comment on lines

+137
 to 
+142

#ifdef _WIN32
 if (preserve_root && key.size() == 3 && key[1] == ':' && key[0] != '/') {
 // If the user gives us C:/ then don't return C:
 return key;
 }
#endif
@lidavidmlidavidmMay 4, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Oh fun.
@westonpace
westonpace
commented
May 4, 2023

Copy link


Copy Markdown

MemberAuthor
There are a few errors but they seem to be unrelated. I'm going to merge this.
@westonpace
westonpace
merged commit 7ca7724
into
apache:mainMay 4, 2023
@ursabot
ursabot
commented
May 5, 2023

Copy link


Copy Markdown

Benchmark runs are scheduled for baseline = febd0ff and contender = 7ca7724. 7ca7724 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.

Conbench compare runs links:

[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2

[Finished ⬇️2.27% ⬆️0.58%] test-mac-arm

[Finished ⬇️0.26% ⬆️0.0%] ursa-i9-9960x

[Finished ⬇️1.96% ⬆️0.93%] ursa-thinkcentre-m75q

Buildkite builds:

[Finished] 7ca77241 ec2-t3-xlarge-us-east-2

[Finished] 7ca77241 test-mac-arm

[Finished] 7ca77241 ursa-i9-9960x

[Finished] 7ca77241 ursa-thinkcentre-m75q

[Finished] febd0ff1 ec2-t3-xlarge-us-east-2

[Finished] febd0ff1 test-mac-arm

[Finished] febd0ff1 ursa-i9-9960x

[Finished] febd0ff1 ursa-thinkcentre-m75q

Supported benchmarks:

ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True

test-mac-arm: Supported benchmark langs: C++, Python, R

ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript

ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
@ursabot
ursabot
commented
May 5, 2023

Copy link


Copy Markdown

['Python', 'R'] benchmarks have high level of regressions.
test-mac-arm
liujiacheng777 pushed a commit
to LoongArch-Python/arrow
that referenced
this pull request
May 11, 2023
@westonpace@liujiacheng777
apacheGH-34386: [C++] Add a PathFromUriOrPath method (apache#34420)
169db77
### Rationale for this change
We have some URI parsing indirectly exposed through FilesystemFromUri. However, this isn't very useful if the user has multiple URIs or if they already have a filesystem. This method allows that same URI handling to be used even if the user already has a filesystem.
### What changes are included in this PR?
Adds a new arrow::fs::PathFromUriOrPath method
### Are these changes tested?
Yes, via unit tests
### Are there any user-facing changes?
There is a new API method but no changes to any existing APIs
* Closes: apache#34386
Authored-by: Weston Pace <weston.pace@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
ArgusLi pushed a commit
to Bit-Quill/arrow
that referenced
this pull request
May 15, 2023
@westonpace@ArgusLi
apacheGH-34386: [C++] Add a PathFromUriOrPath method (apache#34420)
0e727b9
### Rationale for this change
We have some URI parsing indirectly exposed through FilesystemFromUri. However, this isn't very useful if the user has multiple URIs or if they already have a filesystem. This method allows that same URI handling to be used even if the user already has a filesystem.
### What changes are included in this PR?
Adds a new arrow::fs::PathFromUriOrPath method
### Are these changes tested?
Yes, via unit tests
### Are there any user-facing changes?
There is a new API method but no changes to any existing APIs
* Closes: apache#34386
Authored-by: Weston Pace <weston.pace@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@lidavidmlidavidmlidavidm approved these changes

Assignees


No one assigned


Labels

awaiting mergeAwaiting mergeComponent: C++

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

[C++] Handle URI paths for filesystems?


4 participants

@westonpace@lidavidm@amol-@ursabot

, '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
GH-34386: [C++] Add a PathFromUriOrPath method - #34420
Merged
westonpace merged 8 commits into
apache:mainfrom 
westonpace:feature/GH-34386--path-from-urireset
May 4, 2023
Merged
GH-34386: [C++] Add a PathFromUriOrPath method#34420
westonpace merged 8 commits into
apache:mainfrom 
westonpace:feature/GH-34386--path-from-urireset
Conversation
@westonpace
@westonpacewestonpace
commented
Mar 3, 2023•

edited by github-actions
Bot
Loading

Copy link


Copy Markdown

Member
Rationale for this change
We have some URI parsing indirectly exposed through FilesystemFromUri. However, this isn't very useful if the user has multiple URIs or if they already have a filesystem. This method allows that same URI handling to be used even if the user already has a filesystem.
What changes are included in this PR?
Adds a new arrow::fs::PathFromUriOrPath method
Are these changes tested?
Yes, via unit tests
Are there any user-facing changes?
There is a new API method but no changes to any existing APIs
@github-actionsgithub-actionsBot
added
the 
awaiting committer review
Awaiting committer review
label
Mar 3, 2023
@github-actions
github-actionsBot
commented
Mar 3, 2023

Copy link


Copy Markdown

@github-actions
github-actionsBot
commented
Mar 3, 2023

Copy link


Copy Markdown

⚠️ GitHub issue #34386has been automatically assigned in GitHub to PR creator.
@lidavidm
lidavidm
commented
Mar 7, 2023

Copy link


Copy Markdown

Member
Oh neat, thank you. I'll try to take a look at this soon
lidavidm
lidavidm
reviewed
Mar 10, 2023
View reviewed changes
Comment threadcpp/src/arrow/filesystem/filesystem.h
Outdated

Comment threadcpp/src/arrow/filesystem/filesystem.cc
Outdated

Comment threadcpp/src/arrow/filesystem/filesystem.cc
Outdated

@github-actionsgithub-actionsBot
added

awaiting changes
Awaiting changes
and removed

awaiting committer review
Awaiting committer review
labels
Mar 17, 2023
@amol-
amol-
commented
Mar 30, 2023

Copy link


Copy Markdown

Member
Closing because it has been untouched for a while, in case it's still relevant feel free to reopen and move it forward 👍
@amol-amol-
closed this
Mar 30, 2023
@lidavidm
lidavidm
commented
Apr 12, 2023

Copy link


Copy Markdown

Member
Not sure why this was unceremoniously closed. Weston, do you think you would still have time to look at this? It would be useful in #35034
@lidavidmlidavidm
mentioned this pull request
Apr 12, 2023
 Merged

@westonpacewestonpace
reopened this
Apr 14, 2023
@westonpace
westonpace
commented
Apr 14, 2023

Copy link


Copy Markdown

MemberAuthor
Yes, I will try and look at this. I think it still needs some changes.
@westonpace
westonpaceforce-pushed
the
feature/GH-34386--path-from-urireset
branch
from
79dc526 to
9100358CompareMay 2, 2023 16:17
@github-actionsgithub-actionsBot
added

awaiting change review
Awaiting change review
and removed

awaiting changes
Awaiting changes
labels
May 2, 2023
@westonpacewestonpace
mentioned this pull request
May 3, 2023

Closed

@westonpace
westonpace
commented
May 3, 2023

Copy link


Copy Markdown

MemberAuthor
I unified the various paths using a helper method in util_internal.cc. This highlighted one small difference between the various implementations. The s3 and gcs filesystems remove any trailing slash in the path while the hdfs and local filesystems do not. I have opened #35399 for any discussion on that difference but, for the moment, I have changed all the filesystems to always remove the trailing slash for consistency.
@github-actionsgithub-actionsBot
added

awaiting merge
Awaiting merge
and removed

awaiting change review
Awaiting change review
labels
May 3, 2023
@lidavidm
lidavidm
commented
May 3, 2023

Copy link


Copy Markdown

Member
Hmm, I suppose the trailing slash could change listing behavior in S3.
@westonpace
westonpace
commented
May 3, 2023

Copy link


Copy Markdown

MemberAuthor
Hmm, I suppose the trailing slash could change listing behavior in S3.
S3 was already removing the trailing slash so there is no change to the S3 behavior. This only affected the local filesystem.
@lidavidm
lidavidm
commented
May 3, 2023

Copy link


Copy Markdown

Member
Ah, I got that mixed up. LGTM then
@westonpace
westonpaceforce-pushed
the
feature/GH-34386--path-from-urireset
branch
from
da56f29 to
c89e9caCompareMay 4, 2023 15:42
lidavidm
lidavidm
approved these changes
May 4, 2023
View reviewed changes
Comment threadcpp/src/arrow/filesystem/path_util.cc

Comment on lines

+137
 to 
+142

#ifdef _WIN32
 if (preserve_root && key.size() == 3 && key[1] == ':' && key[0] != '/') {
 // If the user gives us C:/ then don't return C:
 return key;
 }
#endif
@lidavidmlidavidmMay 4, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Oh fun.
@westonpace
westonpace
commented
May 4, 2023

Copy link


Copy Markdown

MemberAuthor
There are a few errors but they seem to be unrelated. I'm going to merge this.
@westonpace
westonpace
merged commit 7ca7724
into
apache:mainMay 4, 2023
@ursabot
ursabot
commented
May 5, 2023

Copy link


Copy Markdown

Benchmark runs are scheduled for baseline = febd0ff and contender = 7ca7724. 7ca7724 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.

Conbench compare runs links:

[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2

[Finished ⬇️2.27% ⬆️0.58%] test-mac-arm

[Finished ⬇️0.26% ⬆️0.0%] ursa-i9-9960x

[Finished ⬇️1.96% ⬆️0.93%] ursa-thinkcentre-m75q

Buildkite builds:

[Finished] 7ca77241 ec2-t3-xlarge-us-east-2

[Finished] 7ca77241 test-mac-arm

[Finished] 7ca77241 ursa-i9-9960x

[Finished] 7ca77241 ursa-thinkcentre-m75q

[Finished] febd0ff1 ec2-t3-xlarge-us-east-2

[Finished] febd0ff1 test-mac-arm

[Finished] febd0ff1 ursa-i9-9960x

[Finished] febd0ff1 ursa-thinkcentre-m75q

Supported benchmarks:

ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True

test-mac-arm: Supported benchmark langs: C++, Python, R

ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript

ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
@ursabot
ursabot
commented
May 5, 2023

Copy link


Copy Markdown

['Python', 'R'] benchmarks have high level of regressions.
test-mac-arm
liujiacheng777 pushed a commit
to LoongArch-Python/arrow
that referenced
this pull request
May 11, 2023
@westonpace@liujiacheng777
apacheGH-34386: [C++] Add a PathFromUriOrPath method (apache#34420)
169db77
### Rationale for this change
We have some URI parsing indirectly exposed through FilesystemFromUri. However, this isn't very useful if the user has multiple URIs or if they already have a filesystem. This method allows that same URI handling to be used even if the user already has a filesystem.
### What changes are included in this PR?
Adds a new arrow::fs::PathFromUriOrPath method
### Are these changes tested?
Yes, via unit tests
### Are there any user-facing changes?
There is a new API method but no changes to any existing APIs
* Closes: apache#34386
Authored-by: Weston Pace <weston.pace@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
ArgusLi pushed a commit
to Bit-Quill/arrow
that referenced
this pull request
May 15, 2023
@westonpace@ArgusLi
apacheGH-34386: [C++] Add a PathFromUriOrPath method (apache#34420)
0e727b9
### Rationale for this change
We have some URI parsing indirectly exposed through FilesystemFromUri. However, this isn't very useful if the user has multiple URIs or if they already have a filesystem. This method allows that same URI handling to be used even if the user already has a filesystem.
### What changes are included in this PR?
Adds a new arrow::fs::PathFromUriOrPath method
### Are these changes tested?
Yes, via unit tests
### Are there any user-facing changes?
There is a new API method but no changes to any existing APIs
* Closes: apache#34386
Authored-by: Weston Pace <weston.pace@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@lidavidmlidavidmlidavidm approved these changes

Assignees


No one assigned


Labels

awaiting mergeAwaiting mergeComponent: C++

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

[C++] Handle URI paths for filesystems?


4 participants

@westonpace@lidavidm@amol-@ursabot

, '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
GH-34386: [C++] Add a PathFromUriOrPath method - #34420
Merged
westonpace merged 8 commits into
apache:mainfrom 
westonpace:feature/GH-34386--path-from-urireset
May 4, 2023
Merged
GH-34386: [C++] Add a PathFromUriOrPath method#34420
westonpace merged 8 commits into
apache:mainfrom 
westonpace:feature/GH-34386--path-from-urireset
Conversation
@westonpace
@westonpacewestonpace
commented
Mar 3, 2023•

edited by github-actions
Bot
Loading

Copy link


Copy Markdown

Member
Rationale for this change
We have some URI parsing indirectly exposed through FilesystemFromUri. However, this isn't very useful if the user has multiple URIs or if they already have a filesystem. This method allows that same URI handling to be used even if the user already has a filesystem.
What changes are included in this PR?
Adds a new arrow::fs::PathFromUriOrPath method
Are these changes tested?
Yes, via unit tests
Are there any user-facing changes?
There is a new API method but no changes to any existing APIs
@github-actionsgithub-actionsBot
added
the 
awaiting committer review
Awaiting committer review
label
Mar 3, 2023
@github-actions
github-actionsBot
commented
Mar 3, 2023

Copy link


Copy Markdown

@github-actions
github-actionsBot
commented
Mar 3, 2023

Copy link


Copy Markdown

⚠️ GitHub issue #34386has been automatically assigned in GitHub to PR creator.
@lidavidm
lidavidm
commented
Mar 7, 2023

Copy link


Copy Markdown

Member
Oh neat, thank you. I'll try to take a look at this soon
lidavidm
lidavidm
reviewed
Mar 10, 2023
View reviewed changes
Comment threadcpp/src/arrow/filesystem/filesystem.h
Outdated

Comment threadcpp/src/arrow/filesystem/filesystem.cc
Outdated

Comment threadcpp/src/arrow/filesystem/filesystem.cc
Outdated

@github-actionsgithub-actionsBot
added

awaiting changes
Awaiting changes
and removed

awaiting committer review
Awaiting committer review
labels
Mar 17, 2023
@amol-
amol-
commented
Mar 30, 2023

Copy link


Copy Markdown

Member
Closing because it has been untouched for a while, in case it's still relevant feel free to reopen and move it forward 👍
@amol-amol-
closed this
Mar 30, 2023
@lidavidm
lidavidm
commented
Apr 12, 2023

Copy link


Copy Markdown

Member
Not sure why this was unceremoniously closed. Weston, do you think you would still have time to look at this? It would be useful in #35034
@lidavidmlidavidm
mentioned this pull request
Apr 12, 2023
 Merged

@westonpacewestonpace
reopened this
Apr 14, 2023
@westonpace
westonpace
commented
Apr 14, 2023

Copy link


Copy Markdown

MemberAuthor
Yes, I will try and look at this. I think it still needs some changes.
@westonpace
westonpaceforce-pushed
the
feature/GH-34386--path-from-urireset
branch
from
79dc526 to
9100358CompareMay 2, 2023 16:17
@github-actionsgithub-actionsBot
added

awaiting change review
Awaiting change review
and removed

awaiting changes
Awaiting changes
labels
May 2, 2023
@westonpacewestonpace
mentioned this pull request
May 3, 2023

Closed

@westonpace
westonpace
commented
May 3, 2023

Copy link


Copy Markdown

MemberAuthor
I unified the various paths using a helper method in util_internal.cc. This highlighted one small difference between the various implementations. The s3 and gcs filesystems remove any trailing slash in the path while the hdfs and local filesystems do not. I have opened #35399 for any discussion on that difference but, for the moment, I have changed all the filesystems to always remove the trailing slash for consistency.
@github-actionsgithub-actionsBot
added

awaiting merge
Awaiting merge
and removed

awaiting change review
Awaiting change review
labels
May 3, 2023
@lidavidm
lidavidm
commented
May 3, 2023

Copy link


Copy Markdown

Member
Hmm, I suppose the trailing slash could change listing behavior in S3.
@westonpace
westonpace
commented
May 3, 2023

Copy link


Copy Markdown

MemberAuthor
Hmm, I suppose the trailing slash could change listing behavior in S3.
S3 was already removing the trailing slash so there is no change to the S3 behavior. This only affected the local filesystem.
@lidavidm
lidavidm
commented
May 3, 2023

Copy link


Copy Markdown

Member
Ah, I got that mixed up. LGTM then
@westonpace
westonpaceforce-pushed
the
feature/GH-34386--path-from-urireset
branch
from
da56f29 to
c89e9caCompareMay 4, 2023 15:42
lidavidm
lidavidm
approved these changes
May 4, 2023
View reviewed changes
Comment threadcpp/src/arrow/filesystem/path_util.cc

Comment on lines

+137
 to 
+142

#ifdef _WIN32
 if (preserve_root && key.size() == 3 && key[1] == ':' && key[0] != '/') {
 // If the user gives us C:/ then don't return C:
 return key;
 }
#endif
@lidavidmlidavidmMay 4, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Oh fun.
@westonpace
westonpace
commented
May 4, 2023

Copy link


Copy Markdown

MemberAuthor
There are a few errors but they seem to be unrelated. I'm going to merge this.
@westonpace
westonpace
merged commit 7ca7724
into
apache:mainMay 4, 2023
@ursabot
ursabot
commented
May 5, 2023

Copy link


Copy Markdown

Benchmark runs are scheduled for baseline = febd0ff and contender = 7ca7724. 7ca7724 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.

Conbench compare runs links:

[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2

[Finished ⬇️2.27% ⬆️0.58%] test-mac-arm

[Finished ⬇️0.26% ⬆️0.0%] ursa-i9-9960x

[Finished ⬇️1.96% ⬆️0.93%] ursa-thinkcentre-m75q

Buildkite builds:

[Finished] 7ca77241 ec2-t3-xlarge-us-east-2

[Finished] 7ca77241 test-mac-arm

[Finished] 7ca77241 ursa-i9-9960x

[Finished] 7ca77241 ursa-thinkcentre-m75q

[Finished] febd0ff1 ec2-t3-xlarge-us-east-2

[Finished] febd0ff1 test-mac-arm

[Finished] febd0ff1 ursa-i9-9960x

[Finished] febd0ff1 ursa-thinkcentre-m75q

Supported benchmarks:

ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True

test-mac-arm: Supported benchmark langs: C++, Python, R

ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript

ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
@ursabot
ursabot
commented
May 5, 2023

Copy link


Copy Markdown

['Python', 'R'] benchmarks have high level of regressions.
test-mac-arm
liujiacheng777 pushed a commit
to LoongArch-Python/arrow
that referenced
this pull request
May 11, 2023
@westonpace@liujiacheng777
apacheGH-34386: [C++] Add a PathFromUriOrPath method (apache#34420)
169db77
### Rationale for this change
We have some URI parsing indirectly exposed through FilesystemFromUri. However, this isn't very useful if the user has multiple URIs or if they already have a filesystem. This method allows that same URI handling to be used even if the user already has a filesystem.
### What changes are included in this PR?
Adds a new arrow::fs::PathFromUriOrPath method
### Are these changes tested?
Yes, via unit tests
### Are there any user-facing changes?
There is a new API method but no changes to any existing APIs
* Closes: apache#34386
Authored-by: Weston Pace <weston.pace@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
ArgusLi pushed a commit
to Bit-Quill/arrow
that referenced
this pull request
May 15, 2023
@westonpace@ArgusLi
apacheGH-34386: [C++] Add a PathFromUriOrPath method (apache#34420)
0e727b9
### Rationale for this change
We have some URI parsing indirectly exposed through FilesystemFromUri. However, this isn't very useful if the user has multiple URIs or if they already have a filesystem. This method allows that same URI handling to be used even if the user already has a filesystem.
### What changes are included in this PR?
Adds a new arrow::fs::PathFromUriOrPath method
### Are these changes tested?
Yes, via unit tests
### Are there any user-facing changes?
There is a new API method but no changes to any existing APIs
* Closes: apache#34386
Authored-by: Weston Pace <weston.pace@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@lidavidmlidavidmlidavidm approved these changes

Assignees


No one assigned


Labels

awaiting mergeAwaiting mergeComponent: C++

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

[C++] Handle URI paths for filesystems?


4 participants

@westonpace@lidavidm@amol-@ursabot

, '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
GH-34386: [C++] Add a PathFromUriOrPath method - #34420
Merged
westonpace merged 8 commits into
apache:mainfrom 
westonpace:feature/GH-34386--path-from-urireset
May 4, 2023
Merged
GH-34386: [C++] Add a PathFromUriOrPath method#34420
westonpace merged 8 commits into
apache:mainfrom 
westonpace:feature/GH-34386--path-from-urireset
Conversation
@westonpace
@westonpacewestonpace
commented
Mar 3, 2023•

edited by github-actions
Bot
Loading

Copy link


Copy Markdown

Member
Rationale for this change
We have some URI parsing indirectly exposed through FilesystemFromUri. However, this isn't very useful if the user has multiple URIs or if they already have a filesystem. This method allows that same URI handling to be used even if the user already has a filesystem.
What changes are included in this PR?
Adds a new arrow::fs::PathFromUriOrPath method
Are these changes tested?
Yes, via unit tests
Are there any user-facing changes?
There is a new API method but no changes to any existing APIs
@github-actionsgithub-actionsBot
added
the 
awaiting committer review
Awaiting committer review
label
Mar 3, 2023
@github-actions
github-actionsBot
commented
Mar 3, 2023

Copy link


Copy Markdown

@github-actions
github-actionsBot
commented
Mar 3, 2023

Copy link


Copy Markdown

⚠️ GitHub issue #34386has been automatically assigned in GitHub to PR creator.
@lidavidm
lidavidm
commented
Mar 7, 2023

Copy link


Copy Markdown

Member
Oh neat, thank you. I'll try to take a look at this soon
lidavidm
lidavidm
reviewed
Mar 10, 2023
View reviewed changes
Comment threadcpp/src/arrow/filesystem/filesystem.h
Outdated

Comment threadcpp/src/arrow/filesystem/filesystem.cc
Outdated

Comment threadcpp/src/arrow/filesystem/filesystem.cc
Outdated

@github-actionsgithub-actionsBot
added

awaiting changes
Awaiting changes
and removed

awaiting committer review
Awaiting committer review
labels
Mar 17, 2023
@amol-
amol-
commented
Mar 30, 2023

Copy link


Copy Markdown

Member
Closing because it has been untouched for a while, in case it's still relevant feel free to reopen and move it forward 👍
@amol-amol-
closed this
Mar 30, 2023
@lidavidm
lidavidm
commented
Apr 12, 2023

Copy link


Copy Markdown

Member
Not sure why this was unceremoniously closed. Weston, do you think you would still have time to look at this? It would be useful in #35034
@lidavidmlidavidm
mentioned this pull request
Apr 12, 2023
 Merged

@westonpacewestonpace
reopened this
Apr 14, 2023
@westonpace
westonpace
commented
Apr 14, 2023

Copy link


Copy Markdown

MemberAuthor
Yes, I will try and look at this. I think it still needs some changes.
@westonpace
westonpaceforce-pushed
the
feature/GH-34386--path-from-urireset
branch
from
79dc526 to
9100358CompareMay 2, 2023 16:17
@github-actionsgithub-actionsBot
added

awaiting change review
Awaiting change review
and removed

awaiting changes
Awaiting changes
labels
May 2, 2023
@westonpacewestonpace
mentioned this pull request
May 3, 2023

Closed

@westonpace
westonpace
commented
May 3, 2023

Copy link


Copy Markdown

MemberAuthor
I unified the various paths using a helper method in util_internal.cc. This highlighted one small difference between the various implementations. The s3 and gcs filesystems remove any trailing slash in the path while the hdfs and local filesystems do not. I have opened #35399 for any discussion on that difference but, for the moment, I have changed all the filesystems to always remove the trailing slash for consistency.
@github-actionsgithub-actionsBot
added

awaiting merge
Awaiting merge
and removed

awaiting change review
Awaiting change review
labels
May 3, 2023
@lidavidm
lidavidm
commented
May 3, 2023

Copy link


Copy Markdown

Member
Hmm, I suppose the trailing slash could change listing behavior in S3.
@westonpace
westonpace
commented
May 3, 2023

Copy link


Copy Markdown

MemberAuthor
Hmm, I suppose the trailing slash could change listing behavior in S3.
S3 was already removing the trailing slash so there is no change to the S3 behavior. This only affected the local filesystem.
@lidavidm
lidavidm
commented
May 3, 2023

Copy link


Copy Markdown

Member
Ah, I got that mixed up. LGTM then
@westonpace
westonpaceforce-pushed
the
feature/GH-34386--path-from-urireset
branch
from
da56f29 to
c89e9caCompareMay 4, 2023 15:42
lidavidm
lidavidm
approved these changes
May 4, 2023
View reviewed changes
Comment threadcpp/src/arrow/filesystem/path_util.cc

Comment on lines

+137
 to 
+142

#ifdef _WIN32
 if (preserve_root && key.size() == 3 && key[1] == ':' && key[0] != '/') {
 // If the user gives us C:/ then don't return C:
 return key;
 }
#endif
@lidavidmlidavidmMay 4, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Oh fun.
@westonpace
westonpace
commented
May 4, 2023

Copy link


Copy Markdown

MemberAuthor
There are a few errors but they seem to be unrelated. I'm going to merge this.
@westonpace
westonpace
merged commit 7ca7724
into
apache:mainMay 4, 2023
@ursabot
ursabot
commented
May 5, 2023

Copy link


Copy Markdown

Benchmark runs are scheduled for baseline = febd0ff and contender = 7ca7724. 7ca7724 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.

Conbench compare runs links:

[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2

[Finished ⬇️2.27% ⬆️0.58%] test-mac-arm

[Finished ⬇️0.26% ⬆️0.0%] ursa-i9-9960x

[Finished ⬇️1.96% ⬆️0.93%] ursa-thinkcentre-m75q

Buildkite builds:

[Finished] 7ca77241 ec2-t3-xlarge-us-east-2

[Finished] 7ca77241 test-mac-arm

[Finished] 7ca77241 ursa-i9-9960x

[Finished] 7ca77241 ursa-thinkcentre-m75q

[Finished] febd0ff1 ec2-t3-xlarge-us-east-2

[Finished] febd0ff1 test-mac-arm

[Finished] febd0ff1 ursa-i9-9960x

[Finished] febd0ff1 ursa-thinkcentre-m75q

Supported benchmarks:

ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True

test-mac-arm: Supported benchmark langs: C++, Python, R

ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript

ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
@ursabot
ursabot
commented
May 5, 2023

Copy link


Copy Markdown

['Python', 'R'] benchmarks have high level of regressions.
test-mac-arm
liujiacheng777 pushed a commit
to LoongArch-Python/arrow
that referenced
this pull request
May 11, 2023
@westonpace@liujiacheng777
apacheGH-34386: [C++] Add a PathFromUriOrPath method (apache#34420)
169db77
### Rationale for this change
We have some URI parsing indirectly exposed through FilesystemFromUri. However, this isn't very useful if the user has multiple URIs or if they already have a filesystem. This method allows that same URI handling to be used even if the user already has a filesystem.
### What changes are included in this PR?
Adds a new arrow::fs::PathFromUriOrPath method
### Are these changes tested?
Yes, via unit tests
### Are there any user-facing changes?
There is a new API method but no changes to any existing APIs
* Closes: apache#34386
Authored-by: Weston Pace <weston.pace@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
ArgusLi pushed a commit
to Bit-Quill/arrow
that referenced
this pull request
May 15, 2023
@westonpace@ArgusLi
apacheGH-34386: [C++] Add a PathFromUriOrPath method (apache#34420)
0e727b9
### Rationale for this change
We have some URI parsing indirectly exposed through FilesystemFromUri. However, this isn't very useful if the user has multiple URIs or if they already have a filesystem. This method allows that same URI handling to be used even if the user already has a filesystem.
### What changes are included in this PR?
Adds a new arrow::fs::PathFromUriOrPath method
### Are these changes tested?
Yes, via unit tests
### Are there any user-facing changes?
There is a new API method but no changes to any existing APIs
* Closes: apache#34386
Authored-by: Weston Pace <weston.pace@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@lidavidmlidavidmlidavidm approved these changes

Assignees


No one assigned


Labels

awaiting mergeAwaiting mergeComponent: C++

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

[C++] Handle URI paths for filesystems?


4 participants

@westonpace@lidavidm@amol-@ursabot

, '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
GH-34386: [C++] Add a PathFromUriOrPath method - #34420
Merged
westonpace merged 8 commits into
apache:mainfrom 
westonpace:feature/GH-34386--path-from-urireset
May 4, 2023
Merged
GH-34386: [C++] Add a PathFromUriOrPath method#34420
westonpace merged 8 commits into
apache:mainfrom 
westonpace:feature/GH-34386--path-from-urireset
Conversation
@westonpace
@westonpacewestonpace
commented
Mar 3, 2023•

edited by github-actions
Bot
Loading

Copy link


Copy Markdown

Member
Rationale for this change
We have some URI parsing indirectly exposed through FilesystemFromUri. However, this isn't very useful if the user has multiple URIs or if they already have a filesystem. This method allows that same URI handling to be used even if the user already has a filesystem.
What changes are included in this PR?
Adds a new arrow::fs::PathFromUriOrPath method
Are these changes tested?
Yes, via unit tests
Are there any user-facing changes?
There is a new API method but no changes to any existing APIs
@github-actionsgithub-actionsBot
added
the 
awaiting committer review
Awaiting committer review
label
Mar 3, 2023
@github-actions
github-actionsBot
commented
Mar 3, 2023

Copy link


Copy Markdown

@github-actions
github-actionsBot
commented
Mar 3, 2023

Copy link


Copy Markdown

⚠️ GitHub issue #34386has been automatically assigned in GitHub to PR creator.
@lidavidm
lidavidm
commented
Mar 7, 2023

Copy link


Copy Markdown

Member
Oh neat, thank you. I'll try to take a look at this soon
lidavidm
lidavidm
reviewed
Mar 10, 2023
View reviewed changes
Comment threadcpp/src/arrow/filesystem/filesystem.h
Outdated

Comment threadcpp/src/arrow/filesystem/filesystem.cc
Outdated

Comment threadcpp/src/arrow/filesystem/filesystem.cc
Outdated

@github-actionsgithub-actionsBot
added

awaiting changes
Awaiting changes
and removed

awaiting committer review
Awaiting committer review
labels
Mar 17, 2023
@amol-
amol-
commented
Mar 30, 2023

Copy link


Copy Markdown

Member
Closing because it has been untouched for a while, in case it's still relevant feel free to reopen and move it forward 👍
@amol-amol-
closed this
Mar 30, 2023
@lidavidm
lidavidm
commented
Apr 12, 2023

Copy link


Copy Markdown

Member
Not sure why this was unceremoniously closed. Weston, do you think you would still have time to look at this? It would be useful in #35034
@lidavidmlidavidm
mentioned this pull request
Apr 12, 2023
 Merged

@westonpacewestonpace
reopened this
Apr 14, 2023
@westonpace
westonpace
commented
Apr 14, 2023

Copy link


Copy Markdown

MemberAuthor
Yes, I will try and look at this. I think it still needs some changes.
@westonpace
westonpaceforce-pushed
the
feature/GH-34386--path-from-urireset
branch
from
79dc526 to
9100358CompareMay 2, 2023 16:17
@github-actionsgithub-actionsBot
added

awaiting change review
Awaiting change review
and removed

awaiting changes
Awaiting changes
labels
May 2, 2023
@westonpacewestonpace
mentioned this pull request
May 3, 2023

Closed

@westonpace
westonpace
commented
May 3, 2023

Copy link


Copy Markdown

MemberAuthor
I unified the various paths using a helper method in util_internal.cc. This highlighted one small difference between the various implementations. The s3 and gcs filesystems remove any trailing slash in the path while the hdfs and local filesystems do not. I have opened #35399 for any discussion on that difference but, for the moment, I have changed all the filesystems to always remove the trailing slash for consistency.
@github-actionsgithub-actionsBot
added

awaiting merge
Awaiting merge
and removed

awaiting change review
Awaiting change review
labels
May 3, 2023
@lidavidm
lidavidm
commented
May 3, 2023

Copy link


Copy Markdown

Member
Hmm, I suppose the trailing slash could change listing behavior in S3.
@westonpace
westonpace
commented
May 3, 2023

Copy link


Copy Markdown

MemberAuthor
Hmm, I suppose the trailing slash could change listing behavior in S3.
S3 was already removing the trailing slash so there is no change to the S3 behavior. This only affected the local filesystem.
@lidavidm
lidavidm
commented
May 3, 2023

Copy link


Copy Markdown

Member
Ah, I got that mixed up. LGTM then
@westonpace
westonpaceforce-pushed
the
feature/GH-34386--path-from-urireset
branch
from
da56f29 to
c89e9caCompareMay 4, 2023 15:42
lidavidm
lidavidm
approved these changes
May 4, 2023
View reviewed changes
Comment threadcpp/src/arrow/filesystem/path_util.cc

Comment on lines

+137
 to 
+142

#ifdef _WIN32
 if (preserve_root && key.size() == 3 && key[1] == ':' && key[0] != '/') {
 // If the user gives us C:/ then don't return C:
 return key;
 }
#endif
@lidavidmlidavidmMay 4, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Oh fun.
@westonpace
westonpace
commented
May 4, 2023

Copy link


Copy Markdown

MemberAuthor
There are a few errors but they seem to be unrelated. I'm going to merge this.
@westonpace
westonpace
merged commit 7ca7724
into
apache:mainMay 4, 2023
@ursabot
ursabot
commented
May 5, 2023

Copy link


Copy Markdown

Benchmark runs are scheduled for baseline = febd0ff and contender = 7ca7724. 7ca7724 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.

Conbench compare runs links:

[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2

[Finished ⬇️2.27% ⬆️0.58%] test-mac-arm

[Finished ⬇️0.26% ⬆️0.0%] ursa-i9-9960x

[Finished ⬇️1.96% ⬆️0.93%] ursa-thinkcentre-m75q

Buildkite builds:

[Finished] 7ca77241 ec2-t3-xlarge-us-east-2

[Finished] 7ca77241 test-mac-arm

[Finished] 7ca77241 ursa-i9-9960x

[Finished] 7ca77241 ursa-thinkcentre-m75q

[Finished] febd0ff1 ec2-t3-xlarge-us-east-2

[Finished] febd0ff1 test-mac-arm

[Finished] febd0ff1 ursa-i9-9960x

[Finished] febd0ff1 ursa-thinkcentre-m75q

Supported benchmarks:

ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True

test-mac-arm: Supported benchmark langs: C++, Python, R

ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript

ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
@ursabot
ursabot
commented
May 5, 2023

Copy link


Copy Markdown

['Python', 'R'] benchmarks have high level of regressions.
test-mac-arm
liujiacheng777 pushed a commit
to LoongArch-Python/arrow
that referenced
this pull request
May 11, 2023
@westonpace@liujiacheng777
apacheGH-34386: [C++] Add a PathFromUriOrPath method (apache#34420)
169db77
### Rationale for this change
We have some URI parsing indirectly exposed through FilesystemFromUri. However, this isn't very useful if the user has multiple URIs or if they already have a filesystem. This method allows that same URI handling to be used even if the user already has a filesystem.
### What changes are included in this PR?
Adds a new arrow::fs::PathFromUriOrPath method
### Are these changes tested?
Yes, via unit tests
### Are there any user-facing changes?
There is a new API method but no changes to any existing APIs
* Closes: apache#34386
Authored-by: Weston Pace <weston.pace@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
ArgusLi pushed a commit
to Bit-Quill/arrow
that referenced
this pull request
May 15, 2023
@westonpace@ArgusLi
apacheGH-34386: [C++] Add a PathFromUriOrPath method (apache#34420)
0e727b9
### Rationale for this change
We have some URI parsing indirectly exposed through FilesystemFromUri. However, this isn't very useful if the user has multiple URIs or if they already have a filesystem. This method allows that same URI handling to be used even if the user already has a filesystem.
### What changes are included in this PR?
Adds a new arrow::fs::PathFromUriOrPath method
### Are these changes tested?
Yes, via unit tests
### Are there any user-facing changes?
There is a new API method but no changes to any existing APIs
* Closes: apache#34386
Authored-by: Weston Pace <weston.pace@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@lidavidmlidavidmlidavidm approved these changes

Assignees


No one assigned


Labels

awaiting mergeAwaiting mergeComponent: C++

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

[C++] Handle URI paths for filesystems?


4 participants

@westonpace@lidavidm@amol-@ursabot

, '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
GH-34386: [C++] Add a PathFromUriOrPath method - #34420
Merged
westonpace merged 8 commits into
apache:mainfrom 
westonpace:feature/GH-34386--path-from-urireset
May 4, 2023
Merged
GH-34386: [C++] Add a PathFromUriOrPath method#34420
westonpace merged 8 commits into
apache:mainfrom 
westonpace:feature/GH-34386--path-from-urireset
Conversation
@westonpace
@westonpacewestonpace
commented
Mar 3, 2023•

edited by github-actions
Bot
Loading

Copy link


Copy Markdown

Member
Rationale for this change
We have some URI parsing indirectly exposed through FilesystemFromUri. However, this isn't very useful if the user has multiple URIs or if they already have a filesystem. This method allows that same URI handling to be used even if the user already has a filesystem.
What changes are included in this PR?
Adds a new arrow::fs::PathFromUriOrPath method
Are these changes tested?
Yes, via unit tests
Are there any user-facing changes?
There is a new API method but no changes to any existing APIs
@github-actionsgithub-actionsBot
added
the 
awaiting committer review
Awaiting committer review
label
Mar 3, 2023
@github-actions
github-actionsBot
commented
Mar 3, 2023

Copy link


Copy Markdown

@github-actions
github-actionsBot
commented
Mar 3, 2023

Copy link


Copy Markdown

⚠️ GitHub issue #34386has been automatically assigned in GitHub to PR creator.
@lidavidm
lidavidm
commented
Mar 7, 2023

Copy link


Copy Markdown

Member
Oh neat, thank you. I'll try to take a look at this soon
lidavidm
lidavidm
reviewed
Mar 10, 2023
View reviewed changes
Comment threadcpp/src/arrow/filesystem/filesystem.h
Outdated

Comment threadcpp/src/arrow/filesystem/filesystem.cc
Outdated

Comment threadcpp/src/arrow/filesystem/filesystem.cc
Outdated

@github-actionsgithub-actionsBot
added

awaiting changes
Awaiting changes
and removed

awaiting committer review
Awaiting committer review
labels
Mar 17, 2023
@amol-
amol-
commented
Mar 30, 2023

Copy link


Copy Markdown

Member
Closing because it has been untouched for a while, in case it's still relevant feel free to reopen and move it forward 👍
@amol-amol-
closed this
Mar 30, 2023
@lidavidm
lidavidm
commented
Apr 12, 2023

Copy link


Copy Markdown

Member
Not sure why this was unceremoniously closed. Weston, do you think you would still have time to look at this? It would be useful in #35034
@lidavidmlidavidm
mentioned this pull request
Apr 12, 2023
 Merged

@westonpacewestonpace
reopened this
Apr 14, 2023
@westonpace
westonpace
commented
Apr 14, 2023

Copy link


Copy Markdown

MemberAuthor
Yes, I will try and look at this. I think it still needs some changes.
@westonpace
westonpaceforce-pushed
the
feature/GH-34386--path-from-urireset
branch
from
79dc526 to
9100358CompareMay 2, 2023 16:17
@github-actionsgithub-actionsBot
added

awaiting change review
Awaiting change review
and removed

awaiting changes
Awaiting changes
labels
May 2, 2023
@westonpacewestonpace
mentioned this pull request
May 3, 2023

Closed

@westonpace
westonpace
commented
May 3, 2023

Copy link


Copy Markdown

MemberAuthor
I unified the various paths using a helper method in util_internal.cc. This highlighted one small difference between the various implementations. The s3 and gcs filesystems remove any trailing slash in the path while the hdfs and local filesystems do not. I have opened #35399 for any discussion on that difference but, for the moment, I have changed all the filesystems to always remove the trailing slash for consistency.
@github-actionsgithub-actionsBot
added

awaiting merge
Awaiting merge
and removed

awaiting change review
Awaiting change review
labels
May 3, 2023
@lidavidm
lidavidm
commented
May 3, 2023

Copy link


Copy Markdown

Member
Hmm, I suppose the trailing slash could change listing behavior in S3.
@westonpace
westonpace
commented
May 3, 2023

Copy link


Copy Markdown

MemberAuthor
Hmm, I suppose the trailing slash could change listing behavior in S3.
S3 was already removing the trailing slash so there is no change to the S3 behavior. This only affected the local filesystem.
@lidavidm
lidavidm
commented
May 3, 2023

Copy link


Copy Markdown

Member
Ah, I got that mixed up. LGTM then
@westonpace
westonpaceforce-pushed
the
feature/GH-34386--path-from-urireset
branch
from
da56f29 to
c89e9caCompareMay 4, 2023 15:42
lidavidm
lidavidm
approved these changes
May 4, 2023
View reviewed changes
Comment threadcpp/src/arrow/filesystem/path_util.cc

Comment on lines

+137
 to 
+142

#ifdef _WIN32
 if (preserve_root && key.size() == 3 && key[1] == ':' && key[0] != '/') {
 // If the user gives us C:/ then don't return C:
 return key;
 }
#endif
@lidavidmlidavidmMay 4, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Oh fun.
@westonpace
westonpace
commented
May 4, 2023

Copy link


Copy Markdown

MemberAuthor
There are a few errors but they seem to be unrelated. I'm going to merge this.
@westonpace
westonpace
merged commit 7ca7724
into
apache:mainMay 4, 2023
@ursabot
ursabot
commented
May 5, 2023

Copy link


Copy Markdown

Benchmark runs are scheduled for baseline = febd0ff and contender = 7ca7724. 7ca7724 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.

Conbench compare runs links:

[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2

[Finished ⬇️2.27% ⬆️0.58%] test-mac-arm

[Finished ⬇️0.26% ⬆️0.0%] ursa-i9-9960x

[Finished ⬇️1.96% ⬆️0.93%] ursa-thinkcentre-m75q

Buildkite builds:

[Finished] 7ca77241 ec2-t3-xlarge-us-east-2

[Finished] 7ca77241 test-mac-arm

[Finished] 7ca77241 ursa-i9-9960x

[Finished] 7ca77241 ursa-thinkcentre-m75q

[Finished] febd0ff1 ec2-t3-xlarge-us-east-2

[Finished] febd0ff1 test-mac-arm

[Finished] febd0ff1 ursa-i9-9960x

[Finished] febd0ff1 ursa-thinkcentre-m75q

Supported benchmarks:

ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True

test-mac-arm: Supported benchmark langs: C++, Python, R

ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript

ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
@ursabot
ursabot
commented
May 5, 2023

Copy link


Copy Markdown

['Python', 'R'] benchmarks have high level of regressions.
test-mac-arm
liujiacheng777 pushed a commit
to LoongArch-Python/arrow
that referenced
this pull request
May 11, 2023
@westonpace@liujiacheng777
apacheGH-34386: [C++] Add a PathFromUriOrPath method (apache#34420)
169db77
### Rationale for this change
We have some URI parsing indirectly exposed through FilesystemFromUri. However, this isn't very useful if the user has multiple URIs or if they already have a filesystem. This method allows that same URI handling to be used even if the user already has a filesystem.
### What changes are included in this PR?
Adds a new arrow::fs::PathFromUriOrPath method
### Are these changes tested?
Yes, via unit tests
### Are there any user-facing changes?
There is a new API method but no changes to any existing APIs
* Closes: apache#34386
Authored-by: Weston Pace <weston.pace@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
ArgusLi pushed a commit
to Bit-Quill/arrow
that referenced
this pull request
May 15, 2023
@westonpace@ArgusLi
apacheGH-34386: [C++] Add a PathFromUriOrPath method (apache#34420)
0e727b9
### Rationale for this change
We have some URI parsing indirectly exposed through FilesystemFromUri. However, this isn't very useful if the user has multiple URIs or if they already have a filesystem. This method allows that same URI handling to be used even if the user already has a filesystem.
### What changes are included in this PR?
Adds a new arrow::fs::PathFromUriOrPath method
### Are these changes tested?
Yes, via unit tests
### Are there any user-facing changes?
There is a new API method but no changes to any existing APIs
* Closes: apache#34386
Authored-by: Weston Pace <weston.pace@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment

Reviewers

@lidavidmlidavidmlidavidm approved these changes

Assignees


No one assigned


Labels

awaiting mergeAwaiting mergeComponent: C++

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

[C++] Handle URI paths for filesystems?


4 participants

@westonpace@lidavidm@amol-@ursabot