Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API, Core: Move SQLViewRepresentation to API #9302

Merged
merged 1 commit into from
Dec 14, 2023

Conversation

nastra
Copy link
Contributor

@nastra nastra commented Dec 14, 2023

This moves SQLViewRepresentation from core to the api module mainly so that it's easier usable in the context of https://github.com/apache/iceberg/pull/9247/files#r1419759456

import org.immutables.value.Value;

@Value.Immutable
@Value.Include(value = SQLViewRepresentation.class)
Copy link
Contributor Author

@nastra nastra Dec 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Ok good there's a way then to not duplicate the class and retain backwards compatibility.

@nastra
Copy link
Contributor Author

nastra commented Dec 14, 2023

thanks for the review @amogh-jahagirdar

@nastra nastra merged commit 8572c56 into apache:main Dec 14, 2023
42 checks passed
@nastra nastra deleted the move-sql-view-repr-to-api branch December 14, 2023 16:38
@pvary
Copy link
Contributor

pvary commented Dec 15, 2023

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

@nastra
Copy link
Contributor Author

nastra commented Dec 15, 2023

@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), so I'm curious to know under which circumstances this happens

@pvary
Copy link
Contributor

pvary commented Dec 15, 2023

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 😢

@pvary
Copy link
Contributor

pvary commented Dec 15, 2023

Maybe it was a gradle issue which is fixed by a clean build....

@nastra
Copy link
Contributor Author

nastra commented Dec 15, 2023

@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.

@nastra
Copy link
Contributor Author

nastra commented Dec 22, 2023

@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.

@pvary
Copy link
Contributor

pvary commented Jan 3, 2024

@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

lisirrx pushed a commit to lisirrx/iceberg that referenced this pull request Jan 4, 2024
geruh pushed a commit to geruh/iceberg that referenced this pull request Jan 26, 2024
devangjhabakh pushed a commit to cdouglas/iceberg that referenced this pull request Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants