Re: [PR] Core: Common metadata for TableMetadata and ViewMetadata [iceberg]
nk1506 commented on PR #9682: URL: https://github.com/apache/iceberg/pull/9682#issuecomment-2037762988 Common code abstraction on Hive-Table/View has been parked for later. BaseMetadata for Table and View might not be required. We can fulfil the same using different strategy. -- 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-unsubscr...@iceberg.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Core: Common metadata for TableMetadata and ViewMetadata [iceberg]
nk1506 closed pull request #9682: Core: Common metadata for TableMetadata and ViewMetadata URL: https://github.com/apache/iceberg/pull/9682 -- 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-unsubscr...@iceberg.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Core: Common metadata for TableMetadata and ViewMetadata [iceberg]
szehon-ho commented on PR #9682: URL: https://github.com/apache/iceberg/pull/9682#issuecomment-1984689230 Thanks, i commented on the other thread in https://github.com/apache/iceberg/pull/9852#discussion_r1516373990 the original reason (saw that one first) Can you clarify this concern? > Additionally, this abstraction can actually hide errors, where we check for certain table properties by providing BaseMetadata. These properties don't exist for views, so what will happen if we check TableProperties.ENGINE_HIVE_ENABLED on a table and on view? This is just a class hiearchy. The view now gets methods like String property(String property, String defaultValue), which is just shorthand for viewMetadata.getProperties().getOrDefault(property, defaultValue). Strictly speaking, its not necssary. -- 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-unsubscr...@iceberg.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Core: Common metadata for TableMetadata and ViewMetadata [iceberg]
nastra commented on PR #9682: URL: https://github.com/apache/iceberg/pull/9682#issuecomment-1983818939 I reviewed https://github.com/apache/iceberg/pull/9852/ and also took a look at the metadata abstraction but I'm not convinced it actually makes stuff easier. Most of the time certain Hive methods only require 1-2 params (properties, schema, ...) from table/view metadata, so passing those directly makes more sense. Additionally, this abstraction can actually hide errors, where we check for certain table properties by providing `BaseMetadata`. These properties don't exist for views, so what will happen if we check `TableProperties.ENGINE_HIVE_ENABLED` on a table and on view? -- 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-unsubscr...@iceberg.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Core: Common metadata for TableMetadata and ViewMetadata [iceberg]
nk1506 commented on PR #9682: URL: https://github.com/apache/iceberg/pull/9682#issuecomment-1974868108 > Can we please continue this effort , it has been stalled 2 weeks? > > Let's have a PR that is the end to end version (please not on your own github, but against Iceberg github). Hi @szehon-ho , I have created a new PR here https://github.com/apache/iceberg/pull/9852 . -- 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-unsubscr...@iceberg.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Core: Common metadata for TableMetadata and ViewMetadata [iceberg]
nk1506 commented on PR #9682: URL: https://github.com/apache/iceberg/pull/9682#issuecomment-1971200583 Thanks @szehon-ho for looking into this again. I will raise another combined PR with [CommonMetadata](https://github.com/apache/iceberg/pull/9682), [CommonHiveOperation](https://github.com/apache/iceberg/pull/9461) and [Hive-View](https://github.com/apache/iceberg/pull/8907) -- 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-unsubscr...@iceberg.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Core: Common metadata for TableMetadata and ViewMetadata [iceberg]
szehon-ho commented on PR #9682: URL: https://github.com/apache/iceberg/pull/9682#issuecomment-1970723411 Can we please continue this effort , it has been stalled 2 weeks? Let's have a PR that is the end to end version (please not on your own github, but against Iceberg github). -- 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-unsubscr...@iceberg.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Core: Common metadata for TableMetadata and ViewMetadata [iceberg]
nastra commented on PR #9682: URL: https://github.com/apache/iceberg/pull/9682#issuecomment-1944007197 let's hold off on this one until 1.5.0 is out and the Hive view PR is reviewed so that we can better decide in which direction we'd want to go -- 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-unsubscr...@iceberg.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Core: Common metadata for TableMetadata and ViewMetadata [iceberg]
szehon-ho commented on code in PR #9682:
URL: https://github.com/apache/iceberg/pull/9682#discussion_r1488515592
##
core/src/main/java/org/apache/iceberg/BaseMetadata.java:
##
@@ -0,0 +1,53 @@
+/*
+ * 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;
+
+import java.io.Serializable;
+import java.util.Map;
+import javax.annotation.Nullable;
+import org.apache.iceberg.util.PropertyUtil;
+
+/** A base class for {@link TableMetadata} and {@link
org.apache.iceberg.view.ViewMetadata} */
+public interface BaseMetadata extends Serializable {
+
+ String location();
+
+ Map properties();
+
+ @Nullable
Review Comment:
Slightly prefer not to have it, as most Iceberg classes don't have it. But
understand its from the old class.
--
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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Core: Common metadata for TableMetadata and ViewMetadata [iceberg]
nk1506 commented on code in PR #9682:
URL: https://github.com/apache/iceberg/pull/9682#discussion_r1487100635
##
core/src/main/java/org/apache/iceberg/view/ViewMetadata.java:
##
@@ -68,21 +61,27 @@ default Integer currentSchemaId() {
return currentSchemaId;
}
+ @Value.Parameter(order = 1001)
+ String uuid();
+
+ @Value.Parameter(order = 1002)
+ int formatVersion();
+
Review Comment:
@szehon-ho , Since we are planning to keep common members with ViewMetadata
too with `@Override`. Do we have a need of keeping `@Value.Parameter` in
ViewMetadata. I think only override should works.
WDYT ? I have updated the patch with only `@Override` and removed
`@Value.Parameter` from everywhere.
--
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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Core: Common metadata for TableMetadata and ViewMetadata [iceberg]
szehon-ho commented on code in PR #9682:
URL: https://github.com/apache/iceberg/pull/9682#discussion_r1486600361
##
core/src/main/java/org/apache/iceberg/view/ViewMetadata.java:
##
@@ -68,21 +61,27 @@ default Integer currentSchemaId() {
return currentSchemaId;
}
+ @Value.Parameter(order = 1001)
+ String uuid();
+
+ @Value.Parameter(order = 1002)
+ int formatVersion();
+
Review Comment:
@nk1506 let me know if the question makes more sense.
--
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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Core: Common metadata for TableMetadata and ViewMetadata [iceberg]
szehon-ho commented on code in PR #9682:
URL: https://github.com/apache/iceberg/pull/9682#discussion_r1484625251
##
core/src/main/java/org/apache/iceberg/view/ViewMetadata.java:
##
@@ -68,21 +61,27 @@ default Integer currentSchemaId() {
return currentSchemaId;
}
+ @Value.Parameter(order = 1001)
+ String uuid();
+
+ @Value.Parameter(order = 1002)
+ int formatVersion();
+
Review Comment:
Yea exactly, is it possible to remove @Value.Parameter in the BaseMetadata,
and keep the annotations all in the ViewMetadata.
--
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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Core: Common metadata for TableMetadata and ViewMetadata [iceberg]
nk1506 commented on code in PR #9682:
URL: https://github.com/apache/iceberg/pull/9682#discussion_r1485860581
##
core/src/main/java/org/apache/iceberg/BaseMetadata.java:
##
@@ -0,0 +1,61 @@
+/*
+ * 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;
+
+import java.io.Serializable;
+import java.util.Map;
+import javax.annotation.Nullable;
+import org.apache.iceberg.util.PropertyUtil;
+import org.immutables.value.Value;
+
+/**
+ * A base class for {@link TableMetadata} and {@link
org.apache.iceberg.view.ViewMetadata} It
+ * abstracts out all the common metadata for table ane view.
+ */
+public interface BaseMetadata extends Serializable {
Review Comment:
This is draft [PR](https://github.com/nk1506/iceberg/pull/10/files) . with
https://github.com/apache/iceberg/pull/9461 and
https://github.com/apache/iceberg/pull/9682 .
--
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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Core: Common metadata for TableMetadata and ViewMetadata [iceberg]
szehon-ho commented on code in PR #9682:
URL: https://github.com/apache/iceberg/pull/9682#discussion_r1484631950
##
core/src/main/java/org/apache/iceberg/BaseMetadata.java:
##
@@ -0,0 +1,61 @@
+/*
+ * 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;
+
+import java.io.Serializable;
+import java.util.Map;
+import javax.annotation.Nullable;
+import org.apache.iceberg.util.PropertyUtil;
+import org.immutables.value.Value;
+
+/**
+ * A base class for {@link TableMetadata} and {@link
org.apache.iceberg.view.ViewMetadata} It
+ * abstracts out all the common metadata for table ane view.
+ */
+public interface BaseMetadata extends Serializable {
Review Comment:
Actually sorry the refactor of the Operations is split in
https://github.com/apache/iceberg/pull/9461, #8907 is the whole patch.
--
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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Core: Common metadata for TableMetadata and ViewMetadata [iceberg]
szehon-ho commented on code in PR #9682:
URL: https://github.com/apache/iceberg/pull/9682#discussion_r1484628175
##
core/src/main/java/org/apache/iceberg/BaseMetadata.java:
##
@@ -0,0 +1,61 @@
+/*
+ * 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;
+
+import java.io.Serializable;
+import java.util.Map;
+import javax.annotation.Nullable;
+import org.apache.iceberg.util.PropertyUtil;
+import org.immutables.value.Value;
+
+/**
+ * A base class for {@link TableMetadata} and {@link
org.apache.iceberg.view.ViewMetadata} It
+ * abstracts out all the common metadata for table ane view.
+ */
+public interface BaseMetadata extends Serializable {
Review Comment:
Yea I think the main refactor is over at
https://github.com/apache/iceberg/pull/8907 (including baseOperations and
everything). I do agree that it is a bit hard to read, I have to search over
there to find what is needed and not, and so may miss something :).
--
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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Core: Common metadata for TableMetadata and ViewMetadata [iceberg]
szehon-ho commented on code in PR #9682:
URL: https://github.com/apache/iceberg/pull/9682#discussion_r1484625251
##
core/src/main/java/org/apache/iceberg/view/ViewMetadata.java:
##
@@ -68,21 +61,27 @@ default Integer currentSchemaId() {
return currentSchemaId;
}
+ @Value.Parameter(order = 1001)
+ String uuid();
+
+ @Value.Parameter(order = 1002)
+ int formatVersion();
+
Review Comment:
Yea exactly, will take be able to remove @Value.Parameter in the
BaseMetadata, and keep the annotation all in the ViewMetadata.
--
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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Core: Common metadata for TableMetadata and ViewMetadata [iceberg]
ajantha-bhat commented on code in PR #9682:
URL: https://github.com/apache/iceberg/pull/9682#discussion_r1483911297
##
core/src/main/java/org/apache/iceberg/BaseMetadata.java:
##
@@ -0,0 +1,61 @@
+/*
+ * 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;
+
+import java.io.Serializable;
+import java.util.Map;
+import javax.annotation.Nullable;
+import org.apache.iceberg.util.PropertyUtil;
+import org.immutables.value.Value;
+
+/**
+ * A base class for {@link TableMetadata} and {@link
org.apache.iceberg.view.ViewMetadata} It
+ * abstracts out all the common metadata for table ane view.
+ */
+public interface BaseMetadata extends Serializable {
Review Comment:
we can have a separate draft patch. I think doing that refactoring can help
us know whether the current interfaces in `BaseMetadata` is enough or do we
need more changes here.
--
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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Core: Common metadata for TableMetadata and ViewMetadata [iceberg]
nk1506 commented on code in PR #9682:
URL: https://github.com/apache/iceberg/pull/9682#discussion_r1483884220
##
core/src/main/java/org/apache/iceberg/BaseMetadata.java:
##
@@ -0,0 +1,61 @@
+/*
+ * 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;
+
+import java.io.Serializable;
+import java.util.Map;
+import javax.annotation.Nullable;
+import org.apache.iceberg.util.PropertyUtil;
+import org.immutables.value.Value;
+
+/**
+ * A base class for {@link TableMetadata} and {@link
org.apache.iceberg.view.ViewMetadata} It
+ * abstracts out all the common metadata for table ane view.
+ */
+public interface BaseMetadata extends Serializable {
Review Comment:
I agree, But i think those changes we can do later once this interface
pattern gets approved. Or may be with this patch itself.
WDYT ?
--
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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Core: Common metadata for TableMetadata and ViewMetadata [iceberg]
nk1506 commented on code in PR #9682:
URL: https://github.com/apache/iceberg/pull/9682#discussion_r1483877299
##
core/src/main/java/org/apache/iceberg/view/ViewMetadata.java:
##
@@ -68,21 +61,27 @@ default Integer currentSchemaId() {
return currentSchemaId;
}
+ @Value.Parameter(order = 1001)
+ String uuid();
+
+ @Value.Parameter(order = 1002)
+ int formatVersion();
+
Review Comment:
Could you please add more context? i couldn't understand this part. Do you
mean something like all the Members of BaseMetadata should be with ViewMetadata
too with Override? In that case I dont think we need
` @Value.Parameter(order=?)`
--
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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Core: Common metadata for TableMetadata and ViewMetadata [iceberg]
ajantha-bhat commented on code in PR #9682:
URL: https://github.com/apache/iceberg/pull/9682#discussion_r1483877815
##
core/src/main/java/org/apache/iceberg/BaseMetadata.java:
##
@@ -0,0 +1,61 @@
+/*
+ * 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;
+
+import java.io.Serializable;
+import java.util.Map;
+import javax.annotation.Nullable;
+import org.apache.iceberg.util.PropertyUtil;
+import org.immutables.value.Value;
+
+/**
+ * A base class for {@link TableMetadata} and {@link
org.apache.iceberg.view.ViewMetadata} It
+ * abstracts out all the common metadata for table ane view.
+ */
+public interface BaseMetadata extends Serializable {
Review Comment:
may be we can have an end to end refactoring DRAFT PR and take a decision
based on 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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Core: Common metadata for TableMetadata and ViewMetadata [iceberg]
ajantha-bhat commented on code in PR #9682:
URL: https://github.com/apache/iceberg/pull/9682#discussion_r1483877581
##
core/src/main/java/org/apache/iceberg/BaseMetadata.java:
##
@@ -0,0 +1,61 @@
+/*
+ * 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;
+
+import java.io.Serializable;
+import java.util.Map;
+import javax.annotation.Nullable;
+import org.apache.iceberg.util.PropertyUtil;
+import org.immutables.value.Value;
+
+/**
+ * A base class for {@link TableMetadata} and {@link
org.apache.iceberg.view.ViewMetadata} It
+ * abstracts out all the common metadata for table ane view.
+ */
+public interface BaseMetadata extends Serializable {
Review Comment:
Just one question. Is this refactoring enough?
If we Introduce `BaseMetadata`, I expect the `TableOperations` and
`ViewOperations` to have a base class called `BaseOperations` which uses this
`BaseMetadata` for interfaces like
`void commit(BaseMetadata base, BaseMetadata metadata);`
Without that I don't see much value in this refactoring.
--
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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Core: Common metadata for TableMetadata and ViewMetadata [iceberg]
nk1506 commented on code in PR #9682:
URL: https://github.com/apache/iceberg/pull/9682#discussion_r1483877299
##
core/src/main/java/org/apache/iceberg/view/ViewMetadata.java:
##
@@ -68,21 +61,27 @@ default Integer currentSchemaId() {
return currentSchemaId;
}
+ @Value.Parameter(order = 1001)
+ String uuid();
+
+ @Value.Parameter(order = 1002)
+ int formatVersion();
+
Review Comment:
Could you please add more context? i couldn't understand this part.
--
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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Core: Common metadata for TableMetadata and ViewMetadata [iceberg]
nk1506 commented on code in PR #9682:
URL: https://github.com/apache/iceberg/pull/9682#discussion_r1483876244
##
core/src/main/java/org/apache/iceberg/BaseMetadata.java:
##
@@ -0,0 +1,61 @@
+/*
+ * 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;
+
+import java.io.Serializable;
+import java.util.Map;
+import javax.annotation.Nullable;
+import org.apache.iceberg.util.PropertyUtil;
+import org.immutables.value.Value;
+
+/**
+ * A base class for {@link TableMetadata} and {@link
org.apache.iceberg.view.ViewMetadata} It
+ * abstracts out all the common metadata for table ane view.
+ */
+public interface BaseMetadata extends Serializable {
+
+ @Value.Parameter(order = 1003)
+ String location();
+
+ @Value.Parameter(order = 1008)
+ Map properties();
+
+ @Value.Parameter(order = 1010)
+ @Nullable
+ String metadataFileLocation();
+
+ @Value.Parameter(order = 1011)
+ Schema schema();
Review Comment:
Yes , Schema is needed for Both table and view operations. Ref:
https://github.com/apache/iceberg/pull/8907/files#diff-db46657b84d66e084e15f31b8dab21577efb2ae7102863f94c6c9477782de676R160
--
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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Core: Common metadata for TableMetadata and ViewMetadata [iceberg]
szehon-ho commented on code in PR #9682:
URL: https://github.com/apache/iceberg/pull/9682#discussion_r1483497724
##
core/src/main/java/org/apache/iceberg/BaseMetadata.java:
##
@@ -0,0 +1,61 @@
+/*
+ * 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;
+
+import java.io.Serializable;
+import java.util.Map;
+import javax.annotation.Nullable;
+import org.apache.iceberg.util.PropertyUtil;
+import org.immutables.value.Value;
+
+/**
+ * A base class for {@link TableMetadata} and {@link
org.apache.iceberg.view.ViewMetadata} It
+ * abstracts out all the common metadata for table ane view.
+ */
+public interface BaseMetadata extends Serializable {
+
+ @Value.Parameter(order = 1003)
+ String location();
+
+ @Value.Parameter(order = 1008)
+ Map properties();
+
+ @Value.Parameter(order = 1010)
+ @Nullable
+ String metadataFileLocation();
+
+ @Value.Parameter(order = 1011)
+ Schema schema();
Review Comment:
Is this necessary to make this in the common base class, to allow
ViewOperations ? Should we remove it , if it is not required?
--
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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Core: Common metadata for TableMetadata and ViewMetadata [iceberg]
szehon-ho commented on code in PR #9682:
URL: https://github.com/apache/iceberg/pull/9682#discussion_r1483494509
##
core/src/main/java/org/apache/iceberg/view/ViewMetadata.java:
##
@@ -68,21 +61,27 @@ default Integer currentSchemaId() {
return currentSchemaId;
}
+ @Value.Parameter(order = 1001)
+ String uuid();
+
+ @Value.Parameter(order = 1002)
+ int formatVersion();
+
Review Comment:
Question, can we put Override and Value.parameter here, and remove it from
TableMetadata? It will be more clear.
##
core/src/main/java/org/apache/iceberg/BaseMetadata.java:
##
@@ -0,0 +1,61 @@
+/*
+ * 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;
+
+import java.io.Serializable;
+import java.util.Map;
+import javax.annotation.Nullable;
+import org.apache.iceberg.util.PropertyUtil;
+import org.immutables.value.Value;
+
+/**
+ * A base class for {@link TableMetadata} and {@link
org.apache.iceberg.view.ViewMetadata} It
+ * abstracts out all the common metadata for table ane view.
Review Comment:
typo for ane. I would actually may be renmove the first sentence, it looks
like it does not add that much more info than the first sentence. So just
```
A base class for {@link TableMetadata} and {@link
org.apache.iceberg.view.ViewMetadata}.
```
--
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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] Core: Common metadata for TableMetadata and ViewMetadata [iceberg]
nastra commented on PR #9682: URL: https://github.com/apache/iceberg/pull/9682#issuecomment-1933710499 Sorry for the delay. I have some feedback and I'll take a closer look at this (and the Hive + View support PR) once 1.5.0 is out -- 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-unsubscr...@iceberg.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org
