You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "nastra (via GitHub)" <gi...@apache.org> on 2023/12/14 15:55:43 UTC
[PR] API, Core: Move SQLViewRepresentation to API [iceberg]
nastra opened a new pull request, #9302:
URL: https://github.com/apache/iceberg/pull/9302
(no comment)
--
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
Re: [PR] API, Core: Move SQLViewRepresentation to API [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #9302:
URL: https://github.com/apache/iceberg/pull/9302#issuecomment-1858116202
@pvary thanks for the input, I think that's quite helpful. I'll take a closer look and will provide an update once I know more.
--
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
Re: [PR] API, Core: Move SQLViewRepresentation to API [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #9302:
URL: https://github.com/apache/iceberg/pull/9302#issuecomment-1875336457
@nastra: I think we can skip this for now - still think this should be some caching issue on gradle side, which is very hard to repro so not too many people is affected
--
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
Re: [PR] API, Core: Move SQLViewRepresentation to API [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9302:
URL: https://github.com/apache/iceberg/pull/9302#discussion_r1426926992
##########
core/src/main/java/org/apache/iceberg/view/BaseSQLViewRepresentation.java:
##########
@@ -0,0 +1,30 @@
+/*
+ * 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.view;
+
+import org.immutables.value.Value;
+
+@Value.Immutable
+@Value.Include(value = SQLViewRepresentation.class)
Review Comment:
@amogh-jahagirdar `@Value.Include` is what does the trick. Here's what the Javadoc says:
```
Includes specified abstract value types into generation of processing.
This is usually used to generate immutable implementation of classes
from different packages that source code cannot be changed to place @Value.Immutable.
Only public types of suppored kinds is supported
```
--
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
Re: [PR] API, Core: Move SQLViewRepresentation to API [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #9302:
URL: https://github.com/apache/iceberg/pull/9302#discussion_r1426944012
##########
core/src/main/java/org/apache/iceberg/view/BaseSQLViewRepresentation.java:
##########
@@ -0,0 +1,30 @@
+/*
+ * 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.view;
+
+import org.immutables.value.Value;
+
+@Value.Immutable
+@Value.Include(value = SQLViewRepresentation.class)
Review Comment:
Nice! Ok good there's a way then to not duplicate the class and retain backwards compatibility.
--
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
Re: [PR] API, Core: Move SQLViewRepresentation to API [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #9302:
URL: https://github.com/apache/iceberg/pull/9302#issuecomment-1867599839
@pvary sorry I was not able to reproduce this. I checked out the commit prior to this PR, ran `./gradlew clean build -x test -x integrationTest`, then cherry-picked this commit and ran `./gradlew :iceberg-flink:iceberg-flink-runtime-1.18:build` (without cleaning) and didn't see the issue.
--
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
Re: [PR] API, Core: Move SQLViewRepresentation to API [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #9302:
URL: https://github.com/apache/iceberg/pull/9302
--
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
Re: [PR] API, Core: Move SQLViewRepresentation to API [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #9302:
URL: https://github.com/apache/iceberg/pull/9302#issuecomment-1857964575
Could this be that the following error is caused by this change:
```
# Danger! Multiple jars contain identically named classes. This may cause different behaviour depending on classpath ordering.
# Run ./gradlew checkClassUniqueness --write-locks to update this file
## runtimeClasspath
[org.apache.iceberg:iceberg-api, org.apache.iceberg:iceberg-core]
- org.apache.iceberg.view.SQLViewRepresentation
```
The error could be reproduced by:
```
./gradlew checkClassUniqueness --write-locks
```
--
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
Re: [PR] API, Core: Move SQLViewRepresentation to API [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #9302:
URL: https://github.com/apache/iceberg/pull/9302#issuecomment-1856177487
thanks for the review @amogh-jahagirdar
--
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
Re: [PR] API, Core: Move SQLViewRepresentation to API [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #9302:
URL: https://github.com/apache/iceberg/pull/9302#issuecomment-1858058869
@pvary did you see this error on CI or just when running `./gradlew checkClassUniqueness --write-locks`? I tried to repro this but I'm not able to see that error (I tried JDK8 and JDK18).
--
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
Re: [PR] API, Core: Move SQLViewRepresentation to API [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #9302:
URL: https://github.com/apache/iceberg/pull/9302#issuecomment-1858113490
Maybe it was a gradle issue which is fixed by a clean build....
--
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
Re: [PR] API, Core: Move SQLViewRepresentation to API [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #9302:
URL: https://github.com/apache/iceberg/pull/9302#issuecomment-1858112362
I am using java 8
Previuosly, I have run this:
```
$ ./gradlew :iceberg-flink:iceberg-flink-runtime-1.18:build
Starting a Gradle Daemon, 1 busy Daemon could not be reused, use --status for details
> Task :iceberg-flink:iceberg-flink-runtime-1.18:checkClassUniqueness FAILED
> Task :iceberg-flink:iceberg-flink-1.18:compileTestJava
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: Some input files use unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.
FAILURE: Build failed with an exception.
* What went wrong:
Execution failed for task ':iceberg-flink:iceberg-flink-runtime-1.18:checkClassUniqueness'.
> baseline-class-uniqueness detected multiple jars containing identically named classes. Please resolve these problems, or run `./gradlew checkClassUniqueness --write-locks` to accept them:
# Danger! Multiple jars contain identically named classes. This may cause different behaviour depending on classpath ordering.
# Run ./gradlew checkClassUniqueness --write-locks to update this file
## runtimeClasspath
[org.apache.iceberg:iceberg-api, org.apache.iceberg:iceberg-core]
- org.apache.iceberg.view.SQLViewRepresentation
* Try:
> Run with --stacktrace option to get the stack trace.
> Run with --info or --debug option to get more log output.
> Run with --scan to get full insights.
* Get more help at https://help.gradle.org
Deprecated Gradle features were used in this build, making it incompatible with Gradle 9.0.
You can use '--warning-mode all' to show the individual deprecation warnings and determine if they come from your own scripts or plugins.
See https://docs.gradle.org/8.1.1/userguide/command_line_interface.html#sec:command_line_warnings
BUILD FAILED in 1m 8s
53 actionable tasks: 4 executed, 49 up-to-date
```
I have run the `./gradlew checkClassUniqueness --write-locks` as suggested, and that created plenty of `lock` files:
```
aliyun/baseline-class-uniqueness.lock
arrow/baseline-class-uniqueness.lock
aws/baseline-class-uniqueness.lock
aws/src/main/generated/
azure/baseline-class-uniqueness.lock
core/src/main/generated/
data/baseline-class-uniqueness.lock
dell/baseline-class-uniqueness.lock
delta-lake/baseline-class-uniqueness.lock
flink/v1.18/flink-runtime/baseline-class-uniqueness.lock
flink/v1.18/flink/baseline-class-uniqueness.lock
gcp/baseline-class-uniqueness.lock
hive-metastore/baseline-class-uniqueness.lock
hive-metastore/src/main/generated/
hive-runtime/baseline-class-uniqueness.lock
mr/baseline-class-uniqueness.lock
nessie/baseline-class-uniqueness.lock
orc/baseline-class-uniqueness.lock
parquet/baseline-class-uniqueness.lock
pig/baseline-class-uniqueness.lock
settings-gradle.lockfile
snowflake/baseline-class-uniqueness.lock
spark/v3.5/spark-runtime/baseline-class-uniqueness.lock
spark/v3.5/spark/baseline-class-uniqueness.lock
```
I tried to find the indicated duplicate file, but failed.
Since then I can not repro it either, so just disregard my previous comment.
Sorry for the noise 😢
--
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
Re: [PR] API, Core: Move SQLViewRepresentation to API [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9302:
URL: https://github.com/apache/iceberg/pull/9302#discussion_r1426926992
##########
core/src/main/java/org/apache/iceberg/view/BaseSQLViewRepresentation.java:
##########
@@ -0,0 +1,30 @@
+/*
+ * 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.view;
+
+import org.immutables.value.Value;
+
+@Value.Immutable
+@Value.Include(value = SQLViewRepresentation.class)
Review Comment:
@amogh-jahagirdar `@Value.Include` is what does the trick. Here's what the Javadoc says:
```
Includes specified abstract value types into generation of processing. This is usually used to generate immutable implementation of classes from different packages that source code cannot be changed to place @Value.Immutable. Only public types of suppored kinds is supported
```
--
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