You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/03/01 01:33:03 UTC
[GitHub] [iceberg] kbendick opened a new pull request #4244: Core - Add in RESTUtil class for encoding namespace as URL variable
kbendick opened a new pull request #4244:
URL: https://github.com/apache/iceberg/pull/4244
The REST Catalog uses the string value of a namespace in URLs.
Per the spec, the levels of a multi-level namespace are joined by the NULL byte character (`'\0'` or more correctly `'\u0000'`).
This adds a utility class which can:
1) Properly URL encode a `Namespace` for use as either a path or query parameter
2) Function for decoding the namespace string from a path variable into a Namespace
3) Some helpers for use with REST catalog configuration (function to get all config values that start with a specific prefix, and function to strip the trailing slash from a path if need be).
The items from (3) can likely be put into a shared `CatalogUtil` class, but for now to cause minimal disruption for people who cherry-pick commits into their fork, I'm leaving them as separate.
This PR is to help unblock concurrent work on the REST catalog.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #4244: Core - Add in RESTUtil class for encoding namespace as URL variable
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #4244:
URL: https://github.com/apache/iceberg/pull/4244#discussion_r821034784
##########
File path: core/src/test/java/org/apache/iceberg/rest/TestRESTUtil.java
##########
@@ -0,0 +1,106 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.util.List;
+import java.util.Map;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.assertj.core.api.Assertions;
+import org.junit.Test;
+
+public class TestRESTUtil {
+
+ static class LevelsAndURLEncodedString {
+ public String[] levels;
+ public String parameterEncodedForUrlUsage;
+
+ LevelsAndURLEncodedString(String[] levels, String parameterEncodedForUrlUsage) {
+ this.levels = levels;
+ this.parameterEncodedForUrlUsage = parameterEncodedForUrlUsage;
+ }
+ }
+
+ @Test
+ public void testFilterAndRemovePrefix() {
Review comment:
So for this specific test, I'm actually testing that the whole `input` map, when passed into the function, returns the `expected` map. Including that certain keys (which don't contain the prefix) are removed.
So I'm not sure if using `@ParameterizedTest` here really helps much. It will on `testTrailingSlash` as well as on the `testNamespaceUrlEncodeDecode` method below.
Maybe this is one case where having explicit test case description strings in the `assert` statement would be beneficial.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #4244: Core - Add in RESTUtil class for encoding namespace as URL variable
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #4244:
URL: https://github.com/apache/iceberg/pull/4244#discussion_r821209020
##########
File path: core/src/test/java/org/apache/iceberg/rest/TestRESTUtil.java
##########
@@ -0,0 +1,106 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.util.List;
+import java.util.Map;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.assertj.core.api.Assertions;
+import org.junit.Test;
+
+public class TestRESTUtil {
+
+ static class LevelsAndURLEncodedString {
+ public String[] levels;
+ public String parameterEncodedForUrlUsage;
+
+ LevelsAndURLEncodedString(String[] levels, String parameterEncodedForUrlUsage) {
+ this.levels = levels;
+ this.parameterEncodedForUrlUsage = parameterEncodedForUrlUsage;
+ }
+ }
+
+ @Test
+ public void testFilterAndRemovePrefix() {
Review comment:
Based on another comment, we'll evaluate junit 5 introduction to core possibly in another PR vs in a utilities class PR (for example, in case people don't cherrypick this PR in a fork they maintain etc).
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #4244: Core - Add in RESTUtil class for encoding namespace as URL variable
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #4244:
URL: https://github.com/apache/iceberg/pull/4244#discussion_r821183232
##########
File path: build.gradle
##########
@@ -203,6 +203,10 @@ project(':iceberg-common') {
}
project(':iceberg-core') {
+ test {
+ useJUnitPlatform()
+ }
Review comment:
Ok. Will remove.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #4244: Core - Add in RESTUtil class for encoding namespace as URL variable
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #4244:
URL: https://github.com/apache/iceberg/pull/4244#discussion_r820430437
##########
File path: core/src/main/java/org/apache/iceberg/rest/RESTUtil.java
##########
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.io.UncheckedIOException;
+import java.io.UnsupportedEncodingException;
+import java.net.URLDecoder;
+import java.net.URLEncoder;
+import java.nio.charset.StandardCharsets;
+import java.util.Map;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.relocated.com.google.common.base.Joiner;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.base.Splitter;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.relocated.com.google.common.collect.Streams;
+
+class RESTUtil {
+ private static final Joiner NULL_JOINER = Joiner.on('\u0000');
+ private static final Splitter NULL_SPLITTER = Splitter.on('\u0000');
+
+ private RESTUtil() {
+ }
+
+ public static String stripTrailingSlash(String path) {
+ if (path == null) {
+ return null;
+ }
+
+ return path.endsWith("/") ? path.substring(0, path.length() - 1) : path;
+ }
+
+ public static Map<String, String> filterByPrefix(Map<String, String> properties, String prefix) {
+ Preconditions.checkNotNull(properties, "Invalid properties map: null");
+ Map<String, String> result = Maps.newHashMap();
+ properties.forEach((key, value) -> {
+ if (key.startsWith(prefix)) {
+ result.put(key.substring(prefix.length()), value);
+ }
+ });
+
+ return result;
+ }
+
+ public static String asURLVariable(Namespace ns) {
Review comment:
Added a null check to both (and renamed them). Also added tests for the null checl.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #4244: Core - Add in RESTUtil class for encoding namespace as URL variable
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #4244:
URL: https://github.com/apache/iceberg/pull/4244#discussion_r821183074
##########
File path: core/src/main/java/org/apache/iceberg/rest/RESTUtil.java
##########
@@ -0,0 +1,122 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.io.UncheckedIOException;
+import java.io.UnsupportedEncodingException;
+import java.net.URLDecoder;
+import java.net.URLEncoder;
+import java.nio.charset.StandardCharsets;
+import java.util.Map;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.relocated.com.google.common.base.Joiner;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.base.Splitter;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.relocated.com.google.common.collect.Streams;
+
+class RESTUtil {
+ private static final Joiner NULL_JOINER = Joiner.on('\u0000');
+ private static final Splitter NULL_SPLITTER = Splitter.on('\u0000');
+
+ private RESTUtil() {
+ }
+
+ public static String stripTrailingSlash(String path) {
+ if (path == null) {
+ return null;
+ }
+
+ return path.endsWith("/") ? path.substring(0, path.length() - 1) : path;
+ }
+
+ /**
+ * Takes in a map, and returns a copy with entries with keys beginning with the designated prefix
+ * with the prefix removed. Any entries whose keys don't begin with the prefix are not returned.
+ * <p>
+ * This can be used to get a subset of the configuration related to the REST catalog, such as all
+ * properties from a prefix of `spark.sql.catalog.my_catalog.rest.` to get REST catalog specific properties
+ * from the spark configuration.
+ */
+ public static Map<String, String> filterByPrefix(Map<String, String> properties, String prefix) {
Review comment:
Changed it to extractPrefixMap. We can iterate on the name, but I also updated the doc to try to make it a bit more clear.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #4244: Core - Add in RESTUtil class for encoding namespace as URL variable
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4244:
URL: https://github.com/apache/iceberg/pull/4244#discussion_r821133329
##########
File path: core/src/main/java/org/apache/iceberg/rest/RESTUtil.java
##########
@@ -0,0 +1,122 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.io.UncheckedIOException;
+import java.io.UnsupportedEncodingException;
+import java.net.URLDecoder;
+import java.net.URLEncoder;
+import java.nio.charset.StandardCharsets;
+import java.util.Map;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.relocated.com.google.common.base.Joiner;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.base.Splitter;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.relocated.com.google.common.collect.Streams;
+
+class RESTUtil {
+ private static final Joiner NULL_JOINER = Joiner.on('\u0000');
+ private static final Splitter NULL_SPLITTER = Splitter.on('\u0000');
+
+ private RESTUtil() {
+ }
+
+ public static String stripTrailingSlash(String path) {
+ if (path == null) {
+ return null;
+ }
+
+ return path.endsWith("/") ? path.substring(0, path.length() - 1) : path;
+ }
+
+ /**
+ * Takes in a map, and returns a copy with entries with keys beginning with the designated prefix
+ * with the prefix removed. Any entries whose keys don't begin with the prefix are not returned.
+ * <p>
+ * This can be used to get a subset of the configuration related to the REST catalog, such as all
+ * properties from a prefix of `spark.sql.catalog.my_catalog.rest.` to get REST catalog specific properties
+ * from the spark configuration.
+ */
+ public static Map<String, String> filterByPrefix(Map<String, String> properties, String prefix) {
Review comment:
This name is okay, but it doesn't quite describe what's happening because it is not just filtering by a key prefix, it is also removing the prefix. I wonder if something like `extractPrefixMap` or something would describe it better. I don't really have a good name for it, so it's probably okay as it is.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #4244: Core - Add in RESTUtil class for encoding namespace as URL variable
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #4244:
URL: https://github.com/apache/iceberg/pull/4244#discussion_r820410221
##########
File path: core/src/main/java/org/apache/iceberg/rest/RESTUtil.java
##########
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.io.UncheckedIOException;
+import java.io.UnsupportedEncodingException;
+import java.net.URLDecoder;
+import java.net.URLEncoder;
+import java.nio.charset.StandardCharsets;
+import java.util.Map;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.relocated.com.google.common.base.Joiner;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.base.Splitter;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.relocated.com.google.common.collect.Streams;
+
+class RESTUtil {
+ private static final Joiner NULL_JOINER = Joiner.on('\u0000');
+ private static final Splitter NULL_SPLITTER = Splitter.on('\u0000');
+
+ private RESTUtil() {
+ }
+
+ public static String stripTrailingSlash(String path) {
+ if (path == null) {
+ return null;
+ }
+
+ return path.endsWith("/") ? path.substring(0, path.length() - 1) : path;
+ }
+
+ public static Map<String, String> filterByPrefix(Map<String, String> properties, String prefix) {
Review comment:
That's a good point.
The purpose of this function is to be able to filter out configuration values that are relevant to the catalog, like we do when we configure catalog properties via the spark session configurations. For example, in the jdbc catalog, we configure the following properties.
```bash
--conf spark.sql.catalog.my_catalog.jdbc.verifyServerCertificate=true \
--conf spark.sql.catalog.my_catalog.jdbc.useSSL=true \
--conf spark.sql.catalog.my_catalog.jdbc.user=admin \
--conf spark.sql.catalog.my_catalog.jdbc.password=pass
```
So we'd pass in `spark.sql.catalog.my_catalog.jdbc.` (or just `jdbc.`) to get the configuration values by name.
The same thing could be done for the REST catalog. So here's an example.
```
Map<String, String> props = ImmutableMap.of(spark.sql.catalog.my_catalog.rest.readTimeoutMillis, 5000);
Map<String, String> restCatalogProps = filterByPrefix(props, "spark.sql.catalog.my_catalog.rest.");
```
This function isn't nearly as important as the namespace parsing (which is what's needed more immediately), and arguably this could be achieved inline with guava Maps.filter etc, but we had a bug in the JDBC catalog a bit back because the functionality wasn't properly tested, so continuing to use a dedicated function that is tested seemed smarter. Eventually I'd like to merge them into a simpler `CatalogUtil` or something but right now I'm trying to keep changes more isolated.
Ultimately, we might get rid of this function (and I'd even happily take it out of this PR). I _have_ been using this function in my development process, and it's used in the Jdbc catalog too, but if we want to hold off on merging it I'd be fine with that.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #4244: Core - Add in RESTUtil class for encoding namespace as URL variable
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #4244:
URL: https://github.com/apache/iceberg/pull/4244#discussion_r820488640
##########
File path: core/src/main/java/org/apache/iceberg/rest/RESTUtil.java
##########
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.io.UncheckedIOException;
+import java.io.UnsupportedEncodingException;
+import java.net.URLDecoder;
+import java.net.URLEncoder;
+import java.nio.charset.StandardCharsets;
+import java.util.Map;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.relocated.com.google.common.base.Joiner;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.base.Splitter;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.relocated.com.google.common.collect.Streams;
+
+class RESTUtil {
+ private static final Joiner NULL_JOINER = Joiner.on('\u0000');
+ private static final Splitter NULL_SPLITTER = Splitter.on('\u0000');
+
+ private RESTUtil() {
+ }
+
+ public static String stripTrailingSlash(String path) {
+ if (path == null) {
+ return null;
+ }
+
+ return path.endsWith("/") ? path.substring(0, path.length() - 1) : path;
+ }
+
+ public static Map<String, String> filterByPrefix(Map<String, String> properties, String prefix) {
+ Preconditions.checkNotNull(properties, "Invalid properties map: null");
+ Map<String, String> result = Maps.newHashMap();
+ properties.forEach((key, value) -> {
+ if (key.startsWith(prefix)) {
+ result.put(key.substring(prefix.length()), value);
+ }
+ });
+
+ return result;
+ }
+
+ public static String asURLVariable(Namespace ns) {
+ String[] levels = ns.levels();
+ String[] encodedLevels = new String[levels.length];
+
+ for (int i = 0; i < levels.length; i++) {
+ try {
+ encodedLevels[i] = URLEncoder.encode(levels[i], StandardCharsets.UTF_8.name());
+ } catch (UnsupportedEncodingException e) {
+ throw new UncheckedIOException(
+ String.format("Failed to URL encode namespace as UTF-8 encoding is not supported: %s", ns), e);
+ }
+ }
+
+ return NULL_JOINER.join(encodedLevels);
+ }
+
+ public static Namespace fromURLVariable(String encodedNs) {
+ Iterable<String> encodedLevels = NULL_SPLITTER.split(encodedNs);
Review comment:
Added!
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a change in pull request #4244: Core - Add in RESTUtil class for encoding namespace as URL variable
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #4244:
URL: https://github.com/apache/iceberg/pull/4244#discussion_r820442927
##########
File path: core/src/test/java/org/apache/iceberg/rest/TestRESTUtil.java
##########
@@ -0,0 +1,106 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.util.List;
+import java.util.Map;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.assertj.core.api.Assertions;
+import org.junit.Test;
+
+public class TestRESTUtil {
+
+ static class LevelsAndURLEncodedString {
+ public String[] levels;
+ public String parameterEncodedForUrlUsage;
+
+ LevelsAndURLEncodedString(String[] levels, String parameterEncodedForUrlUsage) {
+ this.levels = levels;
+ this.parameterEncodedForUrlUsage = parameterEncodedForUrlUsage;
+ }
+ }
+
+ @Test
+ public void testFilterAndRemovePrefix() {
Review comment:
no concern at all in terms of performance. I was rather thinking that the code could just look cleaner by separating input & expected result from the actual test code, since that makes reading the code easier.
Note that in case you decide to use JUnit5 features here, you would have to add this to `build.gradle`:
```
project(':iceberg-core') {
test {
useJUnitPlatform()
}
...
}
```
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue merged pull request #4244: Core - Add in RESTUtil class for encoding namespace as URL variable
Posted by GitBox <gi...@apache.org>.
rdblue merged pull request #4244:
URL: https://github.com/apache/iceberg/pull/4244
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #4244: Core - Add in RESTUtil class for encoding namespace as URL variable
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4244:
URL: https://github.com/apache/iceberg/pull/4244#discussion_r821135627
##########
File path: build.gradle
##########
@@ -203,6 +203,10 @@ project(':iceberg-common') {
}
project(':iceberg-core') {
+ test {
+ useJUnitPlatform()
+ }
Review comment:
We have a lot of parameterized tests that don't require JUnit5. Here's an example: https://github.com/apache/iceberg/blob/master/api%2Fsrc%2Ftest%2Fjava%2Forg%2Fapache%2Ficeberg%2Fexpressions%2FTestExpressionSerialization.java#L31-L72
I generally prefer not to change the build in small PRs unless we need to. If we want to move to JUnit5, then we should take a look at that and discuss it on the dev list. My main concern there is the reordering of assert method arguments.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #4244: Core - Add in RESTUtil class for encoding namespace as URL variable
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #4244:
URL: https://github.com/apache/iceberg/pull/4244#discussion_r821183729
##########
File path: core/src/main/java/org/apache/iceberg/rest/RESTUtil.java
##########
@@ -0,0 +1,122 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.io.UncheckedIOException;
+import java.io.UnsupportedEncodingException;
+import java.net.URLDecoder;
+import java.net.URLEncoder;
+import java.nio.charset.StandardCharsets;
+import java.util.Map;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.relocated.com.google.common.base.Joiner;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.base.Splitter;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.relocated.com.google.common.collect.Streams;
+
+class RESTUtil {
+ private static final Joiner NULL_JOINER = Joiner.on('\u0000');
+ private static final Splitter NULL_SPLITTER = Splitter.on('\u0000');
+
+ private RESTUtil() {
+ }
+
+ public static String stripTrailingSlash(String path) {
+ if (path == null) {
+ return null;
+ }
+
+ return path.endsWith("/") ? path.substring(0, path.length() - 1) : path;
+ }
+
+ /**
+ * Takes in a map, and returns a copy with entries with keys beginning with the designated prefix
+ * with the prefix removed. Any entries whose keys don't begin with the prefix are not returned.
+ * <p>
+ * This can be used to get a subset of the configuration related to the REST catalog, such as all
+ * properties from a prefix of `spark.sql.catalog.my_catalog.rest.` to get REST catalog specific properties
+ * from the spark configuration.
+ */
+ public static Map<String, String> filterByPrefix(Map<String, String> properties, String prefix) {
+ Preconditions.checkNotNull(properties, "Invalid properties map: null");
+ Map<String, String> result = Maps.newHashMap();
+ properties.forEach((key, value) -> {
+ if (key != null && key.startsWith(prefix)) {
+ result.put(key.substring(prefix.length()), value);
+ }
+ });
+
+ return result;
+ }
+
+ /**
+ * Returns a String representation of a namespace that is suitable for use in a URL / URI.
+ * <p>
+ * This function needs to be called when a namespace is used as a path variable (or query parameter etc.),
+ * to format the namespace per the spec.
+ * <p>
+ * {@link #urlDecode} should be used to parse the namespace from a URL parameter.
+ * @param ns namespace to encode
+ * @return UTF-8 encoded string representing the namespace, suitable for use as a URL parameter
+ */
+ public static String urlEncode(Namespace ns) {
+ Preconditions.checkArgument(ns != null, "Invalid namespace: null");
+ String[] levels = ns.levels();
+ String[] encodedLevels = new String[levels.length];
+
+ for (int i = 0; i < levels.length; i++) {
+ try {
+ encodedLevels[i] = URLEncoder.encode(levels[i], StandardCharsets.UTF_8.name());
+ } catch (UnsupportedEncodingException e) {
+ throw new UncheckedIOException(
+ String.format("Failed to URL encode namespace '%s' as UTF-8 encoding is not supported", ns), e);
+ }
+ }
+
+ return NULL_JOINER.join(encodedLevels);
+ }
+
+ /**
+ * Takes in a string representation of a namespace as used for a URL parameter
+ * and returns the corresponding namespace.
+ * <p>
+ * See also {@link #urlEncode} for generating correctly formatted URLs.
+ */
+ public static Namespace urlDecode(String encodedNs) {
+ Preconditions.checkArgument(encodedNs != null, "Invalid namespace: null");
+ Iterable<String> encodedLevels = NULL_SPLITTER.split(encodedNs);
+ String[] decodedLevels =
Review comment:
Yeah I agree. I wasn't sure which I cared for more. I have updated to have the same format as `urlEncode`.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #4244: Core - Add in RESTUtil class for encoding namespace as URL variable
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #4244:
URL: https://github.com/apache/iceberg/pull/4244#discussion_r821182787
##########
File path: core/src/main/java/org/apache/iceberg/rest/RESTUtil.java
##########
@@ -0,0 +1,122 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.io.UncheckedIOException;
+import java.io.UnsupportedEncodingException;
+import java.net.URLDecoder;
+import java.net.URLEncoder;
+import java.nio.charset.StandardCharsets;
+import java.util.Map;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.relocated.com.google.common.base.Joiner;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.base.Splitter;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.relocated.com.google.common.collect.Streams;
+
+class RESTUtil {
+ private static final Joiner NULL_JOINER = Joiner.on('\u0000');
+ private static final Splitter NULL_SPLITTER = Splitter.on('\u0000');
+
+ private RESTUtil() {
+ }
+
+ public static String stripTrailingSlash(String path) {
+ if (path == null) {
+ return null;
+ }
+
+ return path.endsWith("/") ? path.substring(0, path.length() - 1) : path;
Review comment:
Updated!
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #4244: Core - Add in RESTUtil class for encoding namespace as URL variable
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #4244:
URL: https://github.com/apache/iceberg/pull/4244#discussion_r819770512
##########
File path: core/src/main/java/org/apache/iceberg/rest/RESTUtil.java
##########
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.io.UncheckedIOException;
+import java.io.UnsupportedEncodingException;
+import java.net.URLDecoder;
+import java.net.URLEncoder;
+import java.nio.charset.StandardCharsets;
+import java.util.Map;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.relocated.com.google.common.base.Joiner;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.base.Splitter;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.relocated.com.google.common.collect.Streams;
+
+class RESTUtil {
+ private static final Joiner NULL_JOINER = Joiner.on('\u0000');
+ private static final Splitter NULL_SPLITTER = Splitter.on('\u0000');
+
+ private RESTUtil() {
+ }
+
+ public static String stripTrailingSlash(String path) {
+ if (path == null) {
+ return null;
+ }
+
+ return path.endsWith("/") ? path.substring(0, path.length() - 1) : path;
+ }
+
+ public static Map<String, String> filterByPrefix(Map<String, String> properties, String prefix) {
+ Preconditions.checkNotNull(properties, "Invalid properties map: null");
+ Map<String, String> result = Maps.newHashMap();
+ properties.forEach((key, value) -> {
+ if (key.startsWith(prefix)) {
+ result.put(key.substring(prefix.length()), value);
+ }
+ });
+
+ return result;
+ }
+
+ public static String asURLVariable(Namespace ns) {
+ String[] levels = ns.levels();
+ String[] encodedLevels = new String[levels.length];
+
+ for (int i = 0; i < levels.length; i++) {
+ try {
+ encodedLevels[i] = URLEncoder.encode(levels[i], StandardCharsets.UTF_8.name());
+ } catch (UnsupportedEncodingException e) {
+ throw new UncheckedIOException(
+ String.format("Failed to URL encode namespace as UTF-8 encoding is not supported: %s", ns), e);
Review comment:
Yeah I agree that reads better. Thanks for the 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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #4244: Core - Add in RESTUtil class for encoding namespace as URL variable
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4244:
URL: https://github.com/apache/iceberg/pull/4244#discussion_r821130586
##########
File path: core/src/main/java/org/apache/iceberg/rest/RESTUtil.java
##########
@@ -0,0 +1,122 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.io.UncheckedIOException;
+import java.io.UnsupportedEncodingException;
+import java.net.URLDecoder;
+import java.net.URLEncoder;
+import java.nio.charset.StandardCharsets;
+import java.util.Map;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.relocated.com.google.common.base.Joiner;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.base.Splitter;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.relocated.com.google.common.collect.Streams;
+
+class RESTUtil {
+ private static final Joiner NULL_JOINER = Joiner.on('\u0000');
+ private static final Splitter NULL_SPLITTER = Splitter.on('\u0000');
+
+ private RESTUtil() {
+ }
+
+ public static String stripTrailingSlash(String path) {
+ if (path == null) {
+ return null;
+ }
+
+ return path.endsWith("/") ? path.substring(0, path.length() - 1) : path;
+ }
+
+ /**
+ * Takes in a map, and returns a copy with entries with keys beginning with the designated prefix
+ * with the prefix removed. Any entries whose keys don't begin with the prefix are not returned.
+ * <p>
+ * This can be used to get a subset of the configuration related to the REST catalog, such as all
+ * properties from a prefix of `spark.sql.catalog.my_catalog.rest.` to get REST catalog specific properties
+ * from the spark configuration.
+ */
+ public static Map<String, String> filterByPrefix(Map<String, String> properties, String prefix) {
+ Preconditions.checkNotNull(properties, "Invalid properties map: null");
+ Map<String, String> result = Maps.newHashMap();
+ properties.forEach((key, value) -> {
+ if (key != null && key.startsWith(prefix)) {
+ result.put(key.substring(prefix.length()), value);
+ }
+ });
+
+ return result;
+ }
+
+ /**
+ * Returns a String representation of a namespace that is suitable for use in a URL / URI.
+ * <p>
+ * This function needs to be called when a namespace is used as a path variable (or query parameter etc.),
+ * to format the namespace per the spec.
+ * <p>
+ * {@link #urlDecode} should be used to parse the namespace from a URL parameter.
+ * @param ns namespace to encode
+ * @return UTF-8 encoded string representing the namespace, suitable for use as a URL parameter
+ */
+ public static String urlEncode(Namespace ns) {
+ Preconditions.checkArgument(ns != null, "Invalid namespace: null");
+ String[] levels = ns.levels();
+ String[] encodedLevels = new String[levels.length];
+
+ for (int i = 0; i < levels.length; i++) {
+ try {
+ encodedLevels[i] = URLEncoder.encode(levels[i], StandardCharsets.UTF_8.name());
+ } catch (UnsupportedEncodingException e) {
+ throw new UncheckedIOException(
+ String.format("Failed to URL encode namespace '%s' as UTF-8 encoding is not supported", ns), e);
+ }
+ }
+
+ return NULL_JOINER.join(encodedLevels);
+ }
+
+ /**
+ * Takes in a string representation of a namespace as used for a URL parameter
+ * and returns the corresponding namespace.
+ * <p>
+ * See also {@link #urlEncode} for generating correctly formatted URLs.
+ */
+ public static Namespace urlDecode(String encodedNs) {
+ Preconditions.checkArgument(encodedNs != null, "Invalid namespace: null");
+ Iterable<String> encodedLevels = NULL_SPLITTER.split(encodedNs);
+ String[] decodedLevels =
Review comment:
Nit: It seems strange that the method above allocates a string and uses a for loop, while this one uses a stream and converts using `toArray`. I usually like to keep methods that work basically the same way consistent so they're easy to understand if you know what is happening in one. I'd probably reformat like the `urlEncode` method.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #4244: Core - Add in RESTUtil class for encoding namespace as URL variable
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #4244:
URL: https://github.com/apache/iceberg/pull/4244#discussion_r821055103
##########
File path: core/src/main/java/org/apache/iceberg/rest/RESTUtil.java
##########
@@ -0,0 +1,122 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.io.UncheckedIOException;
+import java.io.UnsupportedEncodingException;
+import java.net.URLDecoder;
+import java.net.URLEncoder;
+import java.nio.charset.StandardCharsets;
+import java.util.Map;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.relocated.com.google.common.base.Joiner;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.base.Splitter;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.relocated.com.google.common.collect.Streams;
+
+class RESTUtil {
+ private static final Joiner NULL_JOINER = Joiner.on('\u0000');
+ private static final Splitter NULL_SPLITTER = Splitter.on('\u0000');
+
+ private RESTUtil() {
+ }
+
+ public static String stripTrailingSlash(String path) {
+ if (path == null) {
+ return null;
+ }
+
+ return path.endsWith("/") ? path.substring(0, path.length() - 1) : path;
+ }
+
+ /**
+ * Takes in a set of properties, and returns all properties begin with the designated prefix with the prefix
Review comment:
Fixed and reworded.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #4244: Core - Add in RESTUtil class for encoding namespace as URL variable
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #4244:
URL: https://github.com/apache/iceberg/pull/4244#discussion_r821000391
##########
File path: core/src/test/java/org/apache/iceberg/rest/TestRESTUtil.java
##########
@@ -0,0 +1,106 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.util.List;
+import java.util.Map;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.assertj.core.api.Assertions;
+import org.junit.Test;
+
+public class TestRESTUtil {
+
+ static class LevelsAndURLEncodedString {
+ public String[] levels;
+ public String parameterEncodedForUrlUsage;
+
+ LevelsAndURLEncodedString(String[] levels, String parameterEncodedForUrlUsage) {
+ this.levels = levels;
+ this.parameterEncodedForUrlUsage = parameterEncodedForUrlUsage;
+ }
+ }
+
+ @Test
+ public void testFilterAndRemovePrefix() {
Review comment:
Cool will check that out. 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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #4244: Core - Add in RESTUtil class for encoding namespace as URL variable
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #4244:
URL: https://github.com/apache/iceberg/pull/4244#discussion_r820403358
##########
File path: core/src/test/java/org/apache/iceberg/rest/TestRESTUtil.java
##########
@@ -0,0 +1,106 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.util.List;
+import java.util.Map;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.assertj.core.api.Assertions;
+import org.junit.Test;
+
+public class TestRESTUtil {
+
+ static class LevelsAndURLEncodedString {
+ public String[] levels;
+ public String parameterEncodedForUrlUsage;
+
+ LevelsAndURLEncodedString(String[] levels, String parameterEncodedForUrlUsage) {
+ this.levels = levels;
+ this.parameterEncodedForUrlUsage = parameterEncodedForUrlUsage;
+ }
+ }
+
+ @Test
+ public void testFilterAndRemovePrefix() {
Review comment:
Is there a performance concern vs using the `@Parameterized` annotation that instantiates a new test class?
The performance overhead of using `@Parameterized` is why I didn't do that here. I'll look into this. 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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a change in pull request #4244: Core - Add in RESTUtil class for encoding namespace as URL variable
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #4244:
URL: https://github.com/apache/iceberg/pull/4244#discussion_r819376635
##########
File path: core/src/test/java/org/apache/iceberg/rest/TestRESTUtil.java
##########
@@ -0,0 +1,106 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.util.List;
+import java.util.Map;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.assertj.core.api.Assertions;
+import org.junit.Test;
+
+public class TestRESTUtil {
+
+ static class LevelsAndURLEncodedString {
+ public String[] levels;
+ public String parameterEncodedForUrlUsage;
+
+ LevelsAndURLEncodedString(String[] levels, String parameterEncodedForUrlUsage) {
+ this.levels = levels;
+ this.parameterEncodedForUrlUsage = parameterEncodedForUrlUsage;
+ }
+ }
+
+ @Test
+ public void testFilterAndRemovePrefix() {
Review comment:
nit: you could maybe use JUnit5's feature to parameterize test cases (https://junit.org/junit5/docs/current/user-guide/#writing-tests-parameterized-tests), since this would simplify the test and clearly state the input and the expected result. As a quick reference I'm attaching https://github.com/projectnessie/nessie/blob/39f40ced48531001eac11c9233bba1951c01c56d/model/src/test/java/org/projectnessie/model/TestTableReference.java#L52-L59 that shows a quick example how this could be done
##########
File path: core/src/main/java/org/apache/iceberg/rest/RESTUtil.java
##########
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.io.UncheckedIOException;
+import java.io.UnsupportedEncodingException;
+import java.net.URLDecoder;
+import java.net.URLEncoder;
+import java.nio.charset.StandardCharsets;
+import java.util.Map;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.relocated.com.google.common.base.Joiner;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.base.Splitter;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.relocated.com.google.common.collect.Streams;
+
+class RESTUtil {
+ private static final Joiner NULL_JOINER = Joiner.on('\u0000');
+ private static final Splitter NULL_SPLITTER = Splitter.on('\u0000');
+
+ private RESTUtil() {
+ }
+
+ public static String stripTrailingSlash(String path) {
+ if (path == null) {
+ return null;
+ }
+
+ return path.endsWith("/") ? path.substring(0, path.length() - 1) : path;
+ }
+
+ public static Map<String, String> filterByPrefix(Map<String, String> properties, String prefix) {
+ Preconditions.checkNotNull(properties, "Invalid properties map: null");
+ Map<String, String> result = Maps.newHashMap();
+ properties.forEach((key, value) -> {
+ if (key.startsWith(prefix)) {
+ result.put(key.substring(prefix.length()), value);
+ }
+ });
+
+ return result;
+ }
+
+ public static String asURLVariable(Namespace ns) {
Review comment:
nit: maybe just `urlEncode(Namespace ns)` and `urlDecode(String encodedNs)`, since `Variable` feels kind of misleading
##########
File path: core/src/main/java/org/apache/iceberg/rest/RESTUtil.java
##########
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.io.UncheckedIOException;
+import java.io.UnsupportedEncodingException;
+import java.net.URLDecoder;
+import java.net.URLEncoder;
+import java.nio.charset.StandardCharsets;
+import java.util.Map;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.relocated.com.google.common.base.Joiner;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.base.Splitter;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.relocated.com.google.common.collect.Streams;
+
+class RESTUtil {
+ private static final Joiner NULL_JOINER = Joiner.on('\u0000');
+ private static final Splitter NULL_SPLITTER = Splitter.on('\u0000');
+
+ private RESTUtil() {
+ }
+
+ public static String stripTrailingSlash(String path) {
+ if (path == null) {
+ return null;
+ }
+
+ return path.endsWith("/") ? path.substring(0, path.length() - 1) : path;
+ }
+
+ public static Map<String, String> filterByPrefix(Map<String, String> properties, String prefix) {
+ Preconditions.checkNotNull(properties, "Invalid properties map: null");
+ Map<String, String> result = Maps.newHashMap();
+ properties.forEach((key, value) -> {
+ if (key.startsWith(prefix)) {
+ result.put(key.substring(prefix.length()), value);
+ }
+ });
+
+ return result;
+ }
+
+ public static String asURLVariable(Namespace ns) {
+ String[] levels = ns.levels();
+ String[] encodedLevels = new String[levels.length];
+
+ for (int i = 0; i < levels.length; i++) {
+ try {
+ encodedLevels[i] = URLEncoder.encode(levels[i], StandardCharsets.UTF_8.name());
+ } catch (UnsupportedEncodingException e) {
+ throw new UncheckedIOException(
+ String.format("Failed to URL encode namespace as UTF-8 encoding is not supported: %s", ns), e);
+ }
+ }
+
+ return NULL_JOINER.join(encodedLevels);
+ }
+
+ public static Namespace fromURLVariable(String encodedNs) {
+ Iterable<String> encodedLevels = NULL_SPLITTER.split(encodedNs);
Review comment:
nit: maybe add a `null` check for the `encodedNs`
##########
File path: core/src/main/java/org/apache/iceberg/rest/RESTUtil.java
##########
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.io.UncheckedIOException;
+import java.io.UnsupportedEncodingException;
+import java.net.URLDecoder;
+import java.net.URLEncoder;
+import java.nio.charset.StandardCharsets;
+import java.util.Map;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.relocated.com.google.common.base.Joiner;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.base.Splitter;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.relocated.com.google.common.collect.Streams;
+
+class RESTUtil {
+ private static final Joiner NULL_JOINER = Joiner.on('\u0000');
+ private static final Splitter NULL_SPLITTER = Splitter.on('\u0000');
+
+ private RESTUtil() {
+ }
+
+ public static String stripTrailingSlash(String path) {
+ if (path == null) {
+ return null;
+ }
+
+ return path.endsWith("/") ? path.substring(0, path.length() - 1) : path;
+ }
+
+ public static Map<String, String> filterByPrefix(Map<String, String> properties, String prefix) {
Review comment:
nit: maybe add some javadoc to make it clear what the purpose of this is
##########
File path: core/src/main/java/org/apache/iceberg/rest/RESTUtil.java
##########
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.io.UncheckedIOException;
+import java.io.UnsupportedEncodingException;
+import java.net.URLDecoder;
+import java.net.URLEncoder;
+import java.nio.charset.StandardCharsets;
+import java.util.Map;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.relocated.com.google.common.base.Joiner;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.base.Splitter;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.relocated.com.google.common.collect.Streams;
+
+class RESTUtil {
+ private static final Joiner NULL_JOINER = Joiner.on('\u0000');
+ private static final Splitter NULL_SPLITTER = Splitter.on('\u0000');
+
+ private RESTUtil() {
+ }
+
+ public static String stripTrailingSlash(String path) {
+ if (path == null) {
+ return null;
+ }
+
+ return path.endsWith("/") ? path.substring(0, path.length() - 1) : path;
+ }
+
+ public static Map<String, String> filterByPrefix(Map<String, String> properties, String prefix) {
+ Preconditions.checkNotNull(properties, "Invalid properties map: null");
+ Map<String, String> result = Maps.newHashMap();
+ properties.forEach((key, value) -> {
+ if (key.startsWith(prefix)) {
+ result.put(key.substring(prefix.length()), value);
+ }
+ });
+
+ return result;
+ }
+
+ public static String asURLVariable(Namespace ns) {
Review comment:
also I think it would make sense to add a `null` check for the `ns`
##########
File path: core/src/main/java/org/apache/iceberg/rest/RESTUtil.java
##########
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.io.UncheckedIOException;
+import java.io.UnsupportedEncodingException;
+import java.net.URLDecoder;
+import java.net.URLEncoder;
+import java.nio.charset.StandardCharsets;
+import java.util.Map;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.relocated.com.google.common.base.Joiner;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.base.Splitter;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.relocated.com.google.common.collect.Streams;
+
+class RESTUtil {
+ private static final Joiner NULL_JOINER = Joiner.on('\u0000');
+ private static final Splitter NULL_SPLITTER = Splitter.on('\u0000');
+
+ private RESTUtil() {
+ }
+
+ public static String stripTrailingSlash(String path) {
+ if (path == null) {
+ return null;
+ }
+
+ return path.endsWith("/") ? path.substring(0, path.length() - 1) : path;
+ }
+
+ public static Map<String, String> filterByPrefix(Map<String, String> properties, String prefix) {
+ Preconditions.checkNotNull(properties, "Invalid properties map: null");
+ Map<String, String> result = Maps.newHashMap();
+ properties.forEach((key, value) -> {
+ if (key.startsWith(prefix)) {
+ result.put(key.substring(prefix.length()), value);
+ }
+ });
+
+ return result;
+ }
+
+ public static String asURLVariable(Namespace ns) {
+ String[] levels = ns.levels();
+ String[] encodedLevels = new String[levels.length];
+
+ for (int i = 0; i < levels.length; i++) {
+ try {
+ encodedLevels[i] = URLEncoder.encode(levels[i], StandardCharsets.UTF_8.name());
+ } catch (UnsupportedEncodingException e) {
+ throw new UncheckedIOException(
+ String.format("Failed to URL encode namespace as UTF-8 encoding is not supported: %s", ns), e);
Review comment:
nit: wdyt about adding the actual namespace name right in the middle? `Failed to URL encode namespace '%s'as UTF-8 encoding is not supported`
I think this reads a bit more fluent to users running into this error msg, wdyt? Same for the decoding message below maybe
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #4244: Core - Add in RESTUtil class for encoding namespace as URL variable
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #4244:
URL: https://github.com/apache/iceberg/pull/4244#issuecomment-1061295305
Thanks, @kbendick!
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a change in pull request #4244: Core - Add in RESTUtil class for encoding namespace as URL variable
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #4244:
URL: https://github.com/apache/iceberg/pull/4244#discussion_r820676611
##########
File path: core/src/main/java/org/apache/iceberg/rest/RESTUtil.java
##########
@@ -0,0 +1,122 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.io.UncheckedIOException;
+import java.io.UnsupportedEncodingException;
+import java.net.URLDecoder;
+import java.net.URLEncoder;
+import java.nio.charset.StandardCharsets;
+import java.util.Map;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.relocated.com.google.common.base.Joiner;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.base.Splitter;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.relocated.com.google.common.collect.Streams;
+
+class RESTUtil {
+ private static final Joiner NULL_JOINER = Joiner.on('\u0000');
+ private static final Splitter NULL_SPLITTER = Splitter.on('\u0000');
+
+ private RESTUtil() {
+ }
+
+ public static String stripTrailingSlash(String path) {
+ if (path == null) {
+ return null;
+ }
+
+ return path.endsWith("/") ? path.substring(0, path.length() - 1) : path;
+ }
+
+ /**
+ * Takes in a set of properties, and returns all properties begin with the designated prefix with the prefix
Review comment:
nit: begin -> beginning
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #4244: Core - Add in RESTUtil class for encoding namespace as URL variable
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #4244:
URL: https://github.com/apache/iceberg/pull/4244#discussion_r821116987
##########
File path: core/src/main/java/org/apache/iceberg/rest/RESTUtil.java
##########
@@ -0,0 +1,122 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.io.UncheckedIOException;
+import java.io.UnsupportedEncodingException;
+import java.net.URLDecoder;
+import java.net.URLEncoder;
+import java.nio.charset.StandardCharsets;
+import java.util.Map;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.relocated.com.google.common.base.Joiner;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.base.Splitter;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.relocated.com.google.common.collect.Streams;
+
+class RESTUtil {
+ private static final Joiner NULL_JOINER = Joiner.on('\u0000');
+ private static final Splitter NULL_SPLITTER = Splitter.on('\u0000');
+
+ private RESTUtil() {
+ }
+
+ public static String stripTrailingSlash(String path) {
+ if (path == null) {
+ return null;
+ }
+
+ return path.endsWith("/") ? path.substring(0, path.length() - 1) : path;
Review comment:
The version in `LocationProviders` strips all trailing slashes:
```java
private static String stripTrailingSlash(String path) {
String result = path;
while (result.endsWith("/")) {
result = result.substring(0, result.length() - 1);
}
return result;
}
```
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #4244: Core - Add in RESTUtil class for encoding namespace as URL variable
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #4244:
URL: https://github.com/apache/iceberg/pull/4244#discussion_r821034784
##########
File path: core/src/test/java/org/apache/iceberg/rest/TestRESTUtil.java
##########
@@ -0,0 +1,106 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.util.List;
+import java.util.Map;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.assertj.core.api.Assertions;
+import org.junit.Test;
+
+public class TestRESTUtil {
+
+ static class LevelsAndURLEncodedString {
+ public String[] levels;
+ public String parameterEncodedForUrlUsage;
+
+ LevelsAndURLEncodedString(String[] levels, String parameterEncodedForUrlUsage) {
+ this.levels = levels;
+ this.parameterEncodedForUrlUsage = parameterEncodedForUrlUsage;
+ }
+ }
+
+ @Test
+ public void testFilterAndRemovePrefix() {
Review comment:
So for this specific test, I'm actually testing that the whole `input` map, when passed into the function, returns the `expected` map. Including that certain keys (which don't contain the prefix) are removed.
So I'm not sure if using `@ParameterizedTest` here really helps much. It will on `testTrailingSlash` as well as on the `testNamespaceUrlEncodeDecode` method below.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a change in pull request #4244: Core - Add in RESTUtil class for encoding namespace as URL variable
Posted by GitBox <gi...@apache.org>.
nastra commented on a change in pull request #4244:
URL: https://github.com/apache/iceberg/pull/4244#discussion_r820444426
##########
File path: core/src/main/java/org/apache/iceberg/rest/RESTUtil.java
##########
@@ -0,0 +1,95 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg.rest;
+
+import java.io.UncheckedIOException;
+import java.io.UnsupportedEncodingException;
+import java.net.URLDecoder;
+import java.net.URLEncoder;
+import java.nio.charset.StandardCharsets;
+import java.util.Map;
+import org.apache.iceberg.catalog.Namespace;
+import org.apache.iceberg.relocated.com.google.common.base.Joiner;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.base.Splitter;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.relocated.com.google.common.collect.Streams;
+
+class RESTUtil {
+ private static final Joiner NULL_JOINER = Joiner.on('\u0000');
+ private static final Splitter NULL_SPLITTER = Splitter.on('\u0000');
+
+ private RESTUtil() {
+ }
+
+ public static String stripTrailingSlash(String path) {
+ if (path == null) {
+ return null;
+ }
+
+ return path.endsWith("/") ? path.substring(0, path.length() - 1) : path;
+ }
+
+ public static Map<String, String> filterByPrefix(Map<String, String> properties, String prefix) {
Review comment:
I think having your explanation from this comment as a javadoc would be great in case you want to keep the function
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org