ajantha-bhat commented on code in PR #16199:
URL: https://github.com/apache/iceberg/pull/16199#discussion_r3192848299
##########
api/src/main/java/org/apache/iceberg/catalog/SupportsNamespaces.java:
##########
@@ -69,11 +70,42 @@ default void createNamespace(Namespace namespace) {
* method must return ["a"] in the result array.
*
* @return a List of namespace {@link Namespace} names
+ * @deprecated use {@link #listNamespaces(boolean)} method
Review Comment:
We also need to mention the version for removal.
See guidelines here:
https://iceberg.apache.org/contribute/?h=contrib#deprecation-notices
##########
api/src/main/java/org/apache/iceberg/catalog/SupportsNamespaces.java:
##########
@@ -69,11 +70,42 @@ default void createNamespace(Namespace namespace) {
* method must return ["a"] in the result array.
*
* @return a List of namespace {@link Namespace} names
+ * @deprecated use {@link #listNamespaces(boolean)} method
*/
+ @Deprecated
default List<Namespace> listNamespaces() {
return listNamespaces(Namespace.empty());
}
+ /**
+ * List namespaces from the catalog. Returns top-level namespaces by
default. Returns all
+ * namespaces when {@param recursive} is true.
+ *
+ * <p>If an object such as a table, view, or function exists, its parent
namespaces must also
+ * exist and must be returned by this discovery method. For example, if
table a.b.t exists, this
+ * method must return ["a"] in the result array.
+ *
+ * @param recursive if true, list all descendant namespaces, otherwise list
only direct child
+ * namespaces
+ * @return a List of namespace {@link Namespace} names
+ */
+ default List<Namespace> listNamespaces(boolean recursive) {
+ if (!recursive) {
+ return listNamespaces();
Review Comment:
But we just deprecated this method above?
##########
api/src/main/java/org/apache/iceberg/catalog/SupportsNamespaces.java:
##########
@@ -101,9 +133,59 @@ default List<Namespace> listNamespaces() {
*
* @return a List of child {@link Namespace} names from the given namespace
* @throws NoSuchNamespaceException If the namespace does not exist
(optional)
+ * @deprecated use {@link #listNamespaces(Namespace, boolean)}
*/
+ @Deprecated
List<Namespace> listNamespaces(Namespace namespace) throws
NoSuchNamespaceException;
+ /**
+ * List child namespaces from the namespace.
+ *
+ * <p>For two existing tables named 'a.b.c.table' and 'a.b.d.table', this
method returns:
+ *
+ * <ul>
+ * <li>Given: {@code Namespace.empty()}
+ * <li>Returns: {@code Namespace.of("a")}
+ * </ul>
+ *
+ * <ul>
+ * <li>Given: {@code Namespace.of("a")}
+ * <li>Returns: {@code Namespace.of("a", "b")}
+ * </ul>
+ *
+ * <ul>
+ * <li>Given: {@code Namespace.of("a", "b")}
+ * <li>Returns: {@code Namespace.of("a", "b", "c")} and {@code
Namespace.of("a", "b", "d")}
+ * </ul>
+ *
+ * <ul>
+ * <li>Given: {@code Namespace.of("a", "b", "c")}
+ * <li>Returns: empty list, because there are no child namespaces
+ * </ul>
+ *
+ * @param recursive if true, list all descendant namespaces, otherwise list
only direct child
+ * namespaces
+ * @return a List of child {@link Namespace} names from the given namespace
+ * @throws NoSuchNamespaceException If the namespace does not exist
(optional)
+ */
+ default List<Namespace> listNamespaces(Namespace namespace, boolean
recursive)
Review Comment:
non-private method should not have boolean arguments.
https://iceberg.apache.org/contribute/?h=contrib#boolean-arguments
introduce a new method like listNamespacesRecursive(Namespace namespace)
##########
api/src/main/java/org/apache/iceberg/catalog/SupportsNamespaces.java:
##########
@@ -101,9 +133,59 @@ default List<Namespace> listNamespaces() {
*
* @return a List of child {@link Namespace} names from the given namespace
* @throws NoSuchNamespaceException If the namespace does not exist
(optional)
+ * @deprecated use {@link #listNamespaces(Namespace, boolean)}
Review Comment:
We also need to mention the version for removal.
See guidelines here:
https://iceberg.apache.org/contribute/?h=contrib#deprecation-notices
##########
api/src/main/java/org/apache/iceberg/catalog/SupportsNamespaces.java:
##########
@@ -69,11 +70,42 @@ default void createNamespace(Namespace namespace) {
* method must return ["a"] in the result array.
*
* @return a List of namespace {@link Namespace} names
+ * @deprecated use {@link #listNamespaces(boolean)} method
*/
+ @Deprecated
default List<Namespace> listNamespaces() {
return listNamespaces(Namespace.empty());
}
+ /**
+ * List namespaces from the catalog. Returns top-level namespaces by
default. Returns all
+ * namespaces when {@param recursive} is true.
+ *
+ * <p>If an object such as a table, view, or function exists, its parent
namespaces must also
+ * exist and must be returned by this discovery method. For example, if
table a.b.t exists, this
+ * method must return ["a"] in the result array.
+ *
+ * @param recursive if true, list all descendant namespaces, otherwise list
only direct child
+ * namespaces
+ * @return a List of namespace {@link Namespace} names
+ */
+ default List<Namespace> listNamespaces(boolean recursive) {
Review Comment:
non-private method should not have boolean arguments.
https://iceberg.apache.org/contribute/?h=contrib#boolean-arguments
introduce a new method like `listNamespacesRecursive`()
##########
api/src/main/java/org/apache/iceberg/catalog/SupportsNamespaces.java:
##########
@@ -69,11 +70,42 @@ default void createNamespace(Namespace namespace) {
* method must return ["a"] in the result array.
*
* @return a List of namespace {@link Namespace} names
+ * @deprecated use {@link #listNamespaces(boolean)} method
*/
+ @Deprecated
default List<Namespace> listNamespaces() {
return listNamespaces(Namespace.empty());
}
+ /**
+ * List namespaces from the catalog. Returns top-level namespaces by
default. Returns all
Review Comment:
default is not false right? So, what do we mean here "Returns top-level
namespaces by default" ?
##########
api/src/main/java/org/apache/iceberg/catalog/SupportsNamespaces.java:
##########
@@ -101,9 +133,59 @@ default List<Namespace> listNamespaces() {
*
* @return a List of child {@link Namespace} names from the given namespace
* @throws NoSuchNamespaceException If the namespace does not exist
(optional)
+ * @deprecated use {@link #listNamespaces(Namespace, boolean)}
*/
+ @Deprecated
List<Namespace> listNamespaces(Namespace namespace) throws
NoSuchNamespaceException;
+ /**
+ * List child namespaces from the namespace.
+ *
+ * <p>For two existing tables named 'a.b.c.table' and 'a.b.d.table', this
method returns:
+ *
+ * <ul>
+ * <li>Given: {@code Namespace.empty()}
+ * <li>Returns: {@code Namespace.of("a")}
+ * </ul>
+ *
+ * <ul>
+ * <li>Given: {@code Namespace.of("a")}
+ * <li>Returns: {@code Namespace.of("a", "b")}
+ * </ul>
+ *
+ * <ul>
+ * <li>Given: {@code Namespace.of("a", "b")}
+ * <li>Returns: {@code Namespace.of("a", "b", "c")} and {@code
Namespace.of("a", "b", "d")}
+ * </ul>
+ *
+ * <ul>
+ * <li>Given: {@code Namespace.of("a", "b", "c")}
+ * <li>Returns: empty list, because there are no child namespaces
+ * </ul>
+ *
+ * @param recursive if true, list all descendant namespaces, otherwise list
only direct child
+ * namespaces
+ * @return a List of child {@link Namespace} names from the given namespace
+ * @throws NoSuchNamespaceException If the namespace does not exist
(optional)
+ */
+ default List<Namespace> listNamespaces(Namespace namespace, boolean
recursive)
+ throws NoSuchNamespaceException {
+ if (!recursive) {
+ return listNamespaces(namespace);
Review Comment:
But we just deprecated this method above?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
