You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2020/02/13 06:06:08 UTC
[GitHub] [calcite] liyafan82 opened a new pull request #1800: [CALCITE-3790]
Make the URL of FileSource available
liyafan82 opened a new pull request #1800: [CALCITE-3790] Make the URL of FileSource available
URL: https://github.com/apache/calcite/pull/1800
When a FileSource is constructed with only a File object, its URL is left null. This makes it inconvenient for some scenarios where a valid URL is required.
This can be resolved, as each file in the local file system corresponds to a file URL, and we fix it by converting a file object to a file URL.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] liyafan82 commented on a change in pull request #1800:
[CALCITE-3790] Make the URL of FileSource available
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #1800: [CALCITE-3790] Make the URL of FileSource available
URL: https://github.com/apache/calcite/pull/1800#discussion_r380138923
##########
File path: core/src/main/java/org/apache/calcite/util/Sources.java
##########
@@ -167,7 +167,7 @@ private FileSource(URL url) {
private FileSource(File file) {
this.file = Objects.requireNonNull(file);
- this.url = null;
+ this.url = fileToUrl(file);
Review comment:
Yes. It impacts the output of the toString method, when the source is constructed solely by a file object.
I did not find any document specifying the output format, so maybe the client code should not depend on the specific format of the output.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on a change in pull request #1800:
[CALCITE-3790] Make the URL of FileSource available
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #1800: [CALCITE-3790] Make the URL of FileSource available
URL: https://github.com/apache/calcite/pull/1800#discussion_r400710502
##########
File path: core/src/test/java/org/apache/calcite/util/SourceTest.java
##########
@@ -67,6 +70,19 @@ private static String getRootPrefix() {
}
}
+ @Test public void testFileToUrl() throws URISyntaxException {
+ final String path = "abc.txt";
+ Source fileSource = file(null, path);
+ URL url = fileSource.url();
+
+ assertFalse(url == null, "No url generated for file source");
+
+ // convert the url back to a file, and assert they are identical.
+ File urlFile = new File(url.toURI().getPath());
Review comment:
Does this support filenames with spaces?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] liyafan82 commented on issue #1800: [CALCITE-3790] Make
the URL of FileSource available
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on issue #1800: [CALCITE-3790] Make the URL of FileSource available
URL: https://github.com/apache/calcite/pull/1800#issuecomment-590148650
@vlsi I have revised the code to eliminate any behavioral changes. Would you please take another look? Thank you in advance.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] liyafan82 commented on a change in pull request #1800:
[CALCITE-3790] Make the URL of FileSource available
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #1800: [CALCITE-3790] Make the URL of FileSource available
URL: https://github.com/apache/calcite/pull/1800#discussion_r380139171
##########
File path: core/src/test/java/org/apache/calcite/util/SourceTest.java
##########
@@ -67,12 +70,27 @@ private static String getRootPrefix() {
}
}
+ @Test public void testFileToUrl() throws URISyntaxException {
+ final String path = "abc.txt";
+ Source fileSource = file(null, path);
+ URL url = fileSource.url();
+
+ assertFalse(url == null);
Review comment:
Thanks for the good suggestion. I have revised the code to include an error message.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] liyafan82 commented on a change in pull request #1800:
[CALCITE-3790] Make the URL of FileSource available
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #1800: [CALCITE-3790] Make the URL of FileSource available
URL: https://github.com/apache/calcite/pull/1800#discussion_r381099394
##########
File path: core/src/test/java/org/apache/calcite/util/SourceTest.java
##########
@@ -67,12 +70,27 @@ private static String getRootPrefix() {
}
}
+ @Test public void testFileToUrl() throws URISyntaxException {
+ final String path = "abc.txt";
+ Source fileSource = file(null, path);
+ URL url = fileSource.url();
+
+ assertFalse(url == null);
+
+ String urlPath = new File(url.toURI().getPath()).getAbsolutePath();
+ assertEquals(absolutePath(path), urlPath);
+ }
+
+ private String absolutePath(String relativePath) {
+ return new File(relativePath).getAbsolutePath();
+ }
+
@Test public void testAppendWithSpaces() {
String fooRelative = "fo o+";
String fooAbsolute = ROOT_PREFIX + "fo o+";
String barRelative = "b ar+";
String barAbsolute = ROOT_PREFIX + "b ar+";
- assertAppend(file(null, fooRelative), file(null, barRelative), "fo o+/b ar+");
+ assertAppend(file(null, fooRelative), file(null, barRelative), absolutePath("fo o+/b ar+"));
Review comment:
@vlsi
Thanks a lot for the feedback. I agree with you that we should not drop a feature silently.
But sorry I do not fully understand your point.
IMO, we do not drop the support for relative path, as we still can construct a file source with a relative file path, and the source works appropriately. What we change is the internal stuff of the class. So I am not sure what is the feature referred to here.
Maybe the feature refers to the output format of the toString method? I think it is sth. that can be overcome, but I am not sure if it is necessary. It seems the method is only used in the test case, and I do not find any evidence suggesting it is part of the class specification.
Would you please give more information/hints?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on a change in pull request #1800:
[CALCITE-3790] Make the URL of FileSource available
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #1800: [CALCITE-3790] Make the URL of FileSource available
URL: https://github.com/apache/calcite/pull/1800#discussion_r410141941
##########
File path: core/src/main/java/org/apache/calcite/util/Sources.java
##########
@@ -189,14 +198,43 @@ private static File urlToFile(URL url) {
return Paths.get(uri).toFile();
}
+ private static URL fileToUrl(File file) {
+ String filePath = file.getPath();
+ if (!file.isAbsolute()) {
+ // convert relative file paths
+ if (File.separatorChar != '/') {
+ filePath = filePath.replace(File.separatorChar, '/');
+ }
+ if (file.isDirectory() && !filePath.endsWith("/")) {
+ filePath += "/";
+ }
+ try {
+ return new URL(
+ "file", null, 0, URLEncoder.encode(filePath, StandardCharsets.UTF_8.name()));
+ } catch (MalformedURLException e) {
+ throw new IllegalArgumentException("Unable to create URI for file " + filePath, e);
Review comment:
```suggestion
throw new IllegalArgumentException("Unable to create URL for file " + filePath, e);
```
sorry for nit-picking :-/
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on a change in pull request #1800:
[CALCITE-3790] Make the URL of FileSource available
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #1800: [CALCITE-3790] Make the URL of FileSource available
URL: https://github.com/apache/calcite/pull/1800#discussion_r380197228
##########
File path: core/src/test/java/org/apache/calcite/util/SourceTest.java
##########
@@ -67,12 +70,27 @@ private static String getRootPrefix() {
}
}
+ @Test public void testFileToUrl() throws URISyntaxException {
+ final String path = "abc.txt";
+ Source fileSource = file(null, path);
+ URL url = fileSource.url();
+
+ assertFalse(url == null);
+
+ String urlPath = new File(url.toURI().getPath()).getAbsolutePath();
+ assertEquals(absolutePath(path), urlPath);
+ }
+
+ private String absolutePath(String relativePath) {
+ return new File(relativePath).getAbsolutePath();
+ }
+
@Test public void testAppendWithSpaces() {
String fooRelative = "fo o+";
String fooAbsolute = ROOT_PREFIX + "fo o+";
String barRelative = "b ar+";
String barAbsolute = ROOT_PREFIX + "b ar+";
- assertAppend(file(null, fooRelative), file(null, barRelative), "fo o+/b ar+");
+ assertAppend(file(null, fooRelative), file(null, barRelative), absolutePath("fo o+/b ar+"));
Review comment:
> Meantime, the path of the url is changed to an absolute path.
Does that mean the change prevents the use of `FileSource` with relative paths?
What's the reason to always convert the path to absolute format?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] liyafan82 commented on a change in pull request #1800:
[CALCITE-3790] Make the URL of FileSource available
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #1800: [CALCITE-3790] Make the URL of FileSource available
URL: https://github.com/apache/calcite/pull/1800#discussion_r410143730
##########
File path: core/src/main/java/org/apache/calcite/util/Sources.java
##########
@@ -189,14 +198,43 @@ private static File urlToFile(URL url) {
return Paths.get(uri).toFile();
}
+ private static URL fileToUrl(File file) {
+ String filePath = file.getPath();
+ if (!file.isAbsolute()) {
+ // convert relative file paths
+ if (File.separatorChar != '/') {
+ filePath = filePath.replace(File.separatorChar, '/');
+ }
+ if (file.isDirectory() && !filePath.endsWith("/")) {
+ filePath += "/";
+ }
+ try {
+ return new URL(
+ "file", null, 0, URLEncoder.encode(filePath, StandardCharsets.UTF_8.name()));
+ } catch (MalformedURLException e) {
+ throw new IllegalArgumentException("Unable to create URI for file " + filePath, e);
Review comment:
Revised. Thanks a lot for your careful review.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] liyafan82 commented on a change in pull request #1800:
[CALCITE-3790] Make the URL of FileSource available
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #1800: [CALCITE-3790] Make the URL of FileSource available
URL: https://github.com/apache/calcite/pull/1800#discussion_r400793111
##########
File path: core/src/test/java/org/apache/calcite/util/SourceTest.java
##########
@@ -67,6 +70,19 @@ private static String getRootPrefix() {
}
}
+ @Test public void testFileToUrl() throws URISyntaxException {
+ final String path = "abc.txt";
+ Source fileSource = file(null, path);
+ URL url = fileSource.url();
+
+ assertFalse(url == null, "No url generated for file source");
+
+ // convert the url back to a file, and assert they are identical.
+ File urlFile = new File(url.toURI().getPath());
Review comment:
Yes. The standard Java API supports file names with spaces.
I have revised the test case to give a file name with space in it.
Please take a look. Thank you.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on a change in pull request #1800:
[CALCITE-3790] Make the URL of FileSource available
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #1800: [CALCITE-3790] Make the URL of FileSource available
URL: https://github.com/apache/calcite/pull/1800#discussion_r379661222
##########
File path: core/src/test/java/org/apache/calcite/util/SourceTest.java
##########
@@ -67,12 +70,27 @@ private static String getRootPrefix() {
}
}
+ @Test public void testFileToUrl() throws URISyntaxException {
+ final String path = "abc.txt";
+ Source fileSource = file(null, path);
+ URL url = fileSource.url();
+
+ assertFalse(url == null);
Review comment:
Please don't do that. It would be extremely hard to understand the nature of the failure if that assert fails in CI.
Please use the appropriate methods, and add messages so the failure is easy to understand by looking at CI output.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on a change in pull request #1800:
[CALCITE-3790] Make the URL of FileSource available
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #1800: [CALCITE-3790] Make the URL of FileSource available
URL: https://github.com/apache/calcite/pull/1800#discussion_r400707254
##########
File path: core/src/main/java/org/apache/calcite/util/Sources.java
##########
@@ -189,14 +196,25 @@ private static File urlToFile(URL url) {
return Paths.get(uri).toFile();
}
+ private static URL fileToUrl(File file) {
+ URI uri;
+ try {
+ uri = file.toURI();
+ } catch (SecurityException e) {
+ throw new IllegalArgumentException("No access to the underlying file " + file.getPath());
Review comment:
Why do you omit `SecurityException e` in the caused by chain?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] liyafan82 commented on a change in pull request #1800:
[CALCITE-3790] Make the URL of FileSource available
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #1800: [CALCITE-3790] Make the URL of FileSource available
URL: https://github.com/apache/calcite/pull/1800#discussion_r409527315
##########
File path: core/src/main/java/org/apache/calcite/util/Sources.java
##########
@@ -189,14 +196,25 @@ private static File urlToFile(URL url) {
return Paths.get(uri).toFile();
}
+ private static URL fileToUrl(File file) {
Review comment:
@vlsi Thanks a lot for your feedback. I have revised the code accordingly.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] liyafan82 commented on issue #1800: [CALCITE-3790] Make
the URL of FileSource available
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on issue #1800: [CALCITE-3790] Make the URL of FileSource available
URL: https://github.com/apache/calcite/pull/1800#issuecomment-612719922
@vlsi, thanks a lot for your good comments.
I have revised the code accordingly. Do you have any more comments?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on a change in pull request #1800:
[CALCITE-3790] Make the URL of FileSource available
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #1800: [CALCITE-3790] Make the URL of FileSource available
URL: https://github.com/apache/calcite/pull/1800#discussion_r380624971
##########
File path: core/src/test/java/org/apache/calcite/util/SourceTest.java
##########
@@ -67,12 +70,27 @@ private static String getRootPrefix() {
}
}
+ @Test public void testFileToUrl() throws URISyntaxException {
+ final String path = "abc.txt";
+ Source fileSource = file(null, path);
+ URL url = fileSource.url();
+
+ assertFalse(url == null);
+
+ String urlPath = new File(url.toURI().getPath()).getAbsolutePath();
+ assertEquals(absolutePath(path), urlPath);
+ }
+
+ private String absolutePath(String relativePath) {
+ return new File(relativePath).getAbsolutePath();
+ }
+
@Test public void testAppendWithSpaces() {
String fooRelative = "fo o+";
String fooAbsolute = ROOT_PREFIX + "fo o+";
String barRelative = "b ar+";
String barAbsolute = ROOT_PREFIX + "b ar+";
- assertAppend(file(null, fooRelative), file(null, barRelative), "fo o+/b ar+");
+ assertAppend(file(null, fooRelative), file(null, barRelative), absolutePath("fo o+/b ar+"));
Review comment:
Just to recap: the current code supports relative paths.
Now you come and suggest a fix that (silently) drops that support.
I do not like features being dropped for no reason :-/
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on a change in pull request #1800:
[CALCITE-3790] Make the URL of FileSource available
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #1800: [CALCITE-3790] Make the URL of FileSource available
URL: https://github.com/apache/calcite/pull/1800#discussion_r409531157
##########
File path: core/src/test/java/org/apache/calcite/util/SourceTest.java
##########
@@ -67,6 +72,32 @@ private static String getRootPrefix() {
}
}
+ @Test void testRelativeFileToUrl() throws UnsupportedEncodingException {
+ // a file path with space
+ final String path = "abc def.txt";
+ Source fileSource = file(null, path);
+ URL url = fileSource.url();
+
+ assertNotNull(url, "No url generated for file source");
+
+ String expected = "file:" + URLEncoder.encode(path, StandardCharsets.UTF_8.name());
Review comment:
Would you please use literal for the expected value? It would make the expected value easier to understand when reading the code.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi merged pull request #1800: [CALCITE-3790] Make the
url() of Sources.of(file) available
Posted by GitBox <gi...@apache.org>.
vlsi merged pull request #1800: [CALCITE-3790] Make the url() of Sources.of(file) available
URL: https://github.com/apache/calcite/pull/1800
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on a change in pull request #1800:
[CALCITE-3790] Make the URL of FileSource available
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #1800: [CALCITE-3790] Make the URL of FileSource available
URL: https://github.com/apache/calcite/pull/1800#discussion_r379661339
##########
File path: core/src/test/java/org/apache/calcite/util/SourceTest.java
##########
@@ -67,12 +70,27 @@ private static String getRootPrefix() {
}
}
+ @Test public void testFileToUrl() throws URISyntaxException {
+ final String path = "abc.txt";
+ Source fileSource = file(null, path);
+ URL url = fileSource.url();
+
+ assertFalse(url == null);
+
+ String urlPath = new File(url.toURI().getPath()).getAbsolutePath();
+ assertEquals(absolutePath(path), urlPath);
Review comment:
Please add the relevant message to clarify why you expect one thing to be equal to another thing.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on issue #1800: [CALCITE-3790] Make the
URL of FileSource available
Posted by GitBox <gi...@apache.org>.
vlsi commented on issue #1800: [CALCITE-3790] Make the URL of FileSource available
URL: https://github.com/apache/calcite/pull/1800#issuecomment-586487847
It does not look like the PR is ready for commit yet :-/
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on a change in pull request #1800:
[CALCITE-3790] Make the URL of FileSource available
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #1800: [CALCITE-3790] Make the URL of FileSource available
URL: https://github.com/apache/calcite/pull/1800#discussion_r409530515
##########
File path: core/src/main/java/org/apache/calcite/util/Sources.java
##########
@@ -189,14 +198,43 @@ private static File urlToFile(URL url) {
return Paths.get(uri).toFile();
}
+ private static URL fileToUrl(File file) {
+ String filePath = file.getPath();
+ if (!file.isAbsolute()) {
+ // convert relative file paths
+ if (File.separatorChar != '/') {
+ filePath = filePath.replace(File.separatorChar, '/');
+ }
+ if (file.isDirectory() && !filePath.endsWith("/")) {
+ filePath += "/";
+ }
+ try {
+ return new URL(
+ "file", null, 0, URLEncoder.encode(filePath, StandardCharsets.UTF_8.name()));
+ } catch (MalformedURLException e) {
+ throw new IllegalArgumentException("Unable to create URI for file " + filePath, e);
+ } catch (UnsupportedEncodingException e) {
+ throw new IllegalArgumentException("UTF-8 is not supported", e);
+ }
+ } else {
Review comment:
Would you please remove `else`?
It is not really required as `if` branch either returns or throws.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on a change in pull request #1800:
[CALCITE-3790] Make the URL of FileSource available
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #1800: [CALCITE-3790] Make the URL of FileSource available
URL: https://github.com/apache/calcite/pull/1800#discussion_r379662166
##########
File path: core/src/test/java/org/apache/calcite/util/SourceTest.java
##########
@@ -67,12 +70,27 @@ private static String getRootPrefix() {
}
}
+ @Test public void testFileToUrl() throws URISyntaxException {
+ final String path = "abc.txt";
+ Source fileSource = file(null, path);
+ URL url = fileSource.url();
+
+ assertFalse(url == null);
+
+ String urlPath = new File(url.toURI().getPath()).getAbsolutePath();
+ assertEquals(absolutePath(path), urlPath);
+ }
+
+ private String absolutePath(String relativePath) {
+ return new File(relativePath).getAbsolutePath();
+ }
+
@Test public void testAppendWithSpaces() {
String fooRelative = "fo o+";
String fooAbsolute = ROOT_PREFIX + "fo o+";
String barRelative = "b ar+";
String barAbsolute = ROOT_PREFIX + "b ar+";
- assertAppend(file(null, fooRelative), file(null, barRelative), "fo o+/b ar+");
+ assertAppend(file(null, fooRelative), file(null, barRelative), absolutePath("fo o+/b ar+"));
Review comment:
This looks like a behavior change. Can you please clarify what is produced in the output?
Can you please clarify why the append of two relative paths becomes an absolute? It does look weird.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] liyafan82 commented on a change in pull request #1800:
[CALCITE-3790] Make the URL of FileSource available
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #1800: [CALCITE-3790] Make the URL of FileSource available
URL: https://github.com/apache/calcite/pull/1800#discussion_r380144030
##########
File path: core/src/test/java/org/apache/calcite/util/SourceTest.java
##########
@@ -67,12 +70,27 @@ private static String getRootPrefix() {
}
}
+ @Test public void testFileToUrl() throws URISyntaxException {
+ final String path = "abc.txt";
+ Source fileSource = file(null, path);
+ URL url = fileSource.url();
+
+ assertFalse(url == null);
+
+ String urlPath = new File(url.toURI().getPath()).getAbsolutePath();
+ assertEquals(absolutePath(path), urlPath);
Review comment:
Message added. Please take a look. Thank you.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] liyafan82 commented on issue #1800: [CALCITE-3790] Make
the URL of FileSource available
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on issue #1800: [CALCITE-3790] Make the URL of FileSource available
URL: https://github.com/apache/calcite/pull/1800#issuecomment-606382395
@hsyuan Do you have more comments about this PR?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on a change in pull request #1800:
[CALCITE-3790] Make the URL of FileSource available
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #1800: [CALCITE-3790] Make the URL of FileSource available
URL: https://github.com/apache/calcite/pull/1800#discussion_r400710056
##########
File path: core/src/test/java/org/apache/calcite/util/SourceTest.java
##########
@@ -67,6 +70,19 @@ private static String getRootPrefix() {
}
}
+ @Test public void testFileToUrl() throws URISyntaxException {
+ final String path = "abc.txt";
+ Source fileSource = file(null, path);
+ URL url = fileSource.url();
+
+ assertFalse(url == null, "No url generated for file source");
Review comment:
What does this mean?
Could you please remove `== null` from within assert call?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] liyafan82 commented on a change in pull request #1800:
[CALCITE-3790] Make the URL of FileSource available
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #1800: [CALCITE-3790] Make the URL of FileSource available
URL: https://github.com/apache/calcite/pull/1800#discussion_r410139953
##########
File path: core/src/main/java/org/apache/calcite/util/Sources.java
##########
@@ -189,14 +198,43 @@ private static File urlToFile(URL url) {
return Paths.get(uri).toFile();
}
+ private static URL fileToUrl(File file) {
+ String filePath = file.getPath();
+ if (!file.isAbsolute()) {
+ // convert relative file paths
+ if (File.separatorChar != '/') {
+ filePath = filePath.replace(File.separatorChar, '/');
+ }
+ if (file.isDirectory() && !filePath.endsWith("/")) {
+ filePath += "/";
+ }
+ try {
+ return new URL(
+ "file", null, 0, URLEncoder.encode(filePath, StandardCharsets.UTF_8.name()));
+ } catch (MalformedURLException e) {
+ throw new IllegalArgumentException("Unable to create URI for file " + filePath, e);
+ } catch (UnsupportedEncodingException e) {
+ throw new IllegalArgumentException("UTF-8 is not supported", e);
+ }
+ } else {
Review comment:
Removed. Thanks for your suggestion.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] liyafan82 commented on a change in pull request #1800:
[CALCITE-3790] Make the URL of FileSource available
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #1800: [CALCITE-3790] Make the URL of FileSource available
URL: https://github.com/apache/calcite/pull/1800#discussion_r380146713
##########
File path: core/src/test/java/org/apache/calcite/util/SourceTest.java
##########
@@ -67,12 +70,27 @@ private static String getRootPrefix() {
}
}
+ @Test public void testFileToUrl() throws URISyntaxException {
+ final String path = "abc.txt";
+ Source fileSource = file(null, path);
+ URL url = fileSource.url();
+
+ assertFalse(url == null);
+
+ String urlPath = new File(url.toURI().getPath()).getAbsolutePath();
+ assertEquals(absolutePath(path), urlPath);
+ }
+
+ private String absolutePath(String relativePath) {
+ return new File(relativePath).getAbsolutePath();
+ }
+
@Test public void testAppendWithSpaces() {
String fooRelative = "fo o+";
String fooAbsolute = ROOT_PREFIX + "fo o+";
String barRelative = "b ar+";
String barAbsolute = ROOT_PREFIX + "b ar+";
- assertAppend(file(null, fooRelative), file(null, barRelative), "fo o+/b ar+");
+ assertAppend(file(null, fooRelative), file(null, barRelative), absolutePath("fo o+/b ar+"));
Review comment:
Sure. When the file source is constructed solely by a file object, a url is converted from the file object. Meantime, the path of the url is changed to an absolute path. When verifying results, it depends on the toString method of the file source, which uses the url path (as you have observed in a previous comment).
This is why the path has changed to an absolute one.
I have made this explicit by adding a comment in the code.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] liyafan82 commented on issue #1800: [CALCITE-3790] Make
the URL of FileSource available
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on issue #1800: [CALCITE-3790] Make the URL of FileSource available
URL: https://github.com/apache/calcite/pull/1800#issuecomment-606382296
@vlsi The issue indicated by you have been resolved. Do you have any more comments?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on a change in pull request #1800:
[CALCITE-3790] Make the URL of FileSource available
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #1800: [CALCITE-3790] Make the URL of FileSource available
URL: https://github.com/apache/calcite/pull/1800#discussion_r379660732
##########
File path: core/src/main/java/org/apache/calcite/util/Sources.java
##########
@@ -167,7 +167,7 @@ private FileSource(URL url) {
private FileSource(File file) {
this.file = Objects.requireNonNull(file);
- this.url = null;
+ this.url = fileToUrl(file);
Review comment:
This seems to impact `#toString()` output :-/
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on a change in pull request #1800:
[CALCITE-3790] Make the URL of FileSource available
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #1800: [CALCITE-3790] Make the URL of FileSource available
URL: https://github.com/apache/calcite/pull/1800#discussion_r409374467
##########
File path: core/src/main/java/org/apache/calcite/util/Sources.java
##########
@@ -189,14 +196,25 @@ private static File urlToFile(URL url) {
return Paths.get(uri).toFile();
}
+ private static URL fileToUrl(File file) {
Review comment:
I wonder if it should have something like
```java
String filePath = file.getPath();
if (!file.isAbsolute()) {
if (File.separatorChar != '/') {
filePath = filePath.replace(File.separatorChar, '/');
}
if (file.isDirectory() && !filePath.endsWith("/")) {
filePath += "/";
}
try {
return new URL("file", null, 0, URLEncoder.encode(filePath, StandardCharsets.UTF_8.name()));
} catch (MalformedURLException e) {
throw new IllegalArgumentException("Unable to create URI for file " + filePath, e);
} catch (UnsupportedEncodingException e) {
throw new IllegalArgumentException("UTF-8 is not found", e);
}
}
```
Then implementation would handle both relative and absolute files symmetrically.
In other words, for the relative file it would produce something like `file:abc%20def.txt` which is in line with `FileSource(URL)`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] vlsi commented on a change in pull request #1800:
[CALCITE-3790] Make the URL of FileSource available
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #1800: [CALCITE-3790] Make the URL of FileSource available
URL: https://github.com/apache/calcite/pull/1800#discussion_r380624971
##########
File path: core/src/test/java/org/apache/calcite/util/SourceTest.java
##########
@@ -67,12 +70,27 @@ private static String getRootPrefix() {
}
}
+ @Test public void testFileToUrl() throws URISyntaxException {
+ final String path = "abc.txt";
+ Source fileSource = file(null, path);
+ URL url = fileSource.url();
+
+ assertFalse(url == null);
+
+ String urlPath = new File(url.toURI().getPath()).getAbsolutePath();
+ assertEquals(absolutePath(path), urlPath);
+ }
+
+ private String absolutePath(String relativePath) {
+ return new File(relativePath).getAbsolutePath();
+ }
+
@Test public void testAppendWithSpaces() {
String fooRelative = "fo o+";
String fooAbsolute = ROOT_PREFIX + "fo o+";
String barRelative = "b ar+";
String barAbsolute = ROOT_PREFIX + "b ar+";
- assertAppend(file(null, fooRelative), file(null, barRelative), "fo o+/b ar+");
+ assertAppend(file(null, fooRelative), file(null, barRelative), absolutePath("fo o+/b ar+"));
Review comment:
Just to recap: the current code supports relative paths.
Now you came and suggest a fix that (silently) drops that support.
I do not like features being dropped for no reason :-/
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] liyafan82 commented on a change in pull request #1800:
[CALCITE-3790] Make the URL of FileSource available
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #1800: [CALCITE-3790] Make the URL of FileSource available
URL: https://github.com/apache/calcite/pull/1800#discussion_r400792462
##########
File path: core/src/test/java/org/apache/calcite/util/SourceTest.java
##########
@@ -67,6 +70,19 @@ private static String getRootPrefix() {
}
}
+ @Test public void testFileToUrl() throws URISyntaxException {
+ final String path = "abc.txt";
+ Source fileSource = file(null, path);
+ URL url = fileSource.url();
+
+ assertFalse(url == null, "No url generated for file source");
Review comment:
Thank you for the good suggestion.
I have revised the code accordingly. Please take anotehr look.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] liyafan82 commented on a change in pull request #1800:
[CALCITE-3790] Make the URL of FileSource available
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #1800: [CALCITE-3790] Make the URL of FileSource available
URL: https://github.com/apache/calcite/pull/1800#discussion_r410140204
##########
File path: core/src/test/java/org/apache/calcite/util/SourceTest.java
##########
@@ -67,6 +72,32 @@ private static String getRootPrefix() {
}
}
+ @Test void testRelativeFileToUrl() throws UnsupportedEncodingException {
+ // a file path with space
+ final String path = "abc def.txt";
+ Source fileSource = file(null, path);
+ URL url = fileSource.url();
+
+ assertNotNull(url, "No url generated for file source");
+
+ String expected = "file:" + URLEncoder.encode(path, StandardCharsets.UTF_8.name());
Review comment:
Sure. Thank you for the good suggestion.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] liyafan82 commented on a change in pull request #1800:
[CALCITE-3790] Make the URL of FileSource available
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #1800: [CALCITE-3790] Make the URL of FileSource available
URL: https://github.com/apache/calcite/pull/1800#discussion_r380463602
##########
File path: core/src/test/java/org/apache/calcite/util/SourceTest.java
##########
@@ -67,12 +70,27 @@ private static String getRootPrefix() {
}
}
+ @Test public void testFileToUrl() throws URISyntaxException {
+ final String path = "abc.txt";
+ Source fileSource = file(null, path);
+ URL url = fileSource.url();
+
+ assertFalse(url == null);
+
+ String urlPath = new File(url.toURI().getPath()).getAbsolutePath();
+ assertEquals(absolutePath(path), urlPath);
+ }
+
+ private String absolutePath(String relativePath) {
+ return new File(relativePath).getAbsolutePath();
+ }
+
@Test public void testAppendWithSpaces() {
String fooRelative = "fo o+";
String fooAbsolute = ROOT_PREFIX + "fo o+";
String barRelative = "b ar+";
String barAbsolute = ROOT_PREFIX + "b ar+";
- assertAppend(file(null, fooRelative), file(null, barRelative), "fo o+/b ar+");
+ assertAppend(file(null, fooRelative), file(null, barRelative), absolutePath("fo o+/b ar+"));
Review comment:
> Does that mean the change prevents the use of FileSource with relative paths?
Maybe not. We can still provide a relative path to the constructor of FileSource. The constructor will convert it to an absolute path and use it to construct a url. FileSource still works appropriately.
> What's the reason to always convert the path to absolute format?
This is determined by the behavior of the standard Java API. But I think the fundamental reason is that, according to the definition (https://en.wikipedia.org/wiki/URL), a URL is always based on an absolute path, as it needs to identify a resource without the aid of other information (e.g. working directory).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [calcite] liyafan82 commented on a change in pull request #1800:
[CALCITE-3790] Make the URL of FileSource available
Posted by GitBox <gi...@apache.org>.
liyafan82 commented on a change in pull request #1800: [CALCITE-3790] Make the URL of FileSource available
URL: https://github.com/apache/calcite/pull/1800#discussion_r400792118
##########
File path: core/src/main/java/org/apache/calcite/util/Sources.java
##########
@@ -189,14 +196,25 @@ private static File urlToFile(URL url) {
return Paths.get(uri).toFile();
}
+ private static URL fileToUrl(File file) {
+ URI uri;
+ try {
+ uri = file.toURI();
+ } catch (SecurityException e) {
+ throw new IllegalArgumentException("No access to the underlying file " + file.getPath());
Review comment:
Nice catch. Thank you.
I have revised the code to wrap the SecurityException in the IllegalArgumentException.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services