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

test(java): add Checker Framework annotations #1495

Merged
merged 1 commit into from
Feb 9, 2024
Merged

Conversation

lidavidm
Copy link
Member

Fixes #624.

@lidavidm lidavidm marked this pull request as ready for review February 8, 2024 20:47
@lidavidm
Copy link
Member Author

lidavidm commented Feb 8, 2024

CC @jduo @davisusanibar

@lidavidm lidavidm changed the title test(java): add Checker Framework test(java): add Checker Framework annotations Feb 8, 2024
@@ -145,7 +150,12 @@ VectorSchemaRoot build() throws AdbcException {

root.setRowCount(dstIndex);
VectorSchemaRoot result = root;
root = null;
try {
root = VectorSchemaRoot.create(StandardSchemas.GET_INFO_SCHEMA, allocator);
Copy link
Member

Choose a reason for hiding this comment

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

Why does an empty root get recreated here?

FYI I'm reworking some of this code for the getObjects() impl. Planning to make this class an Iterator and have RootArrowReader consume the iterator to avoid front-loading the client when there are lots of metadata objects.

Copy link
Member Author

Choose a reason for hiding this comment

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

The class overall is AutoCloseable, and when it is closed it will clear the root. So in this method, we need to "transfer ownership" of the root out. Instead of dealing with making the root nullable, I decided to just give it an empty root always.

@@ -85,27 +85,27 @@ protected void timestamp4WithoutTimeZoneType() throws Exception {
protected void timestamp3WithoutTimeZoneType() throws Exception {
final Schema schema = connection.getTableSchema(null, null, "adbc_alltypes");
assertThat(schema.findField("timestamp_without_time_zone_p3_t").getType())
.isEqualTo(new ArrowType.Timestamp(TimeUnit.MICROSECOND, null));
.isEqualTo(new ArrowType.Timestamp(TimeUnit.MILLISECOND, null));
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure how this change is related to the CheckerFramework.

Copy link
Member Author

Choose a reason for hiding this comment

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

I noticed there were two different type mapping functions and unified on the more comprehensive one.

Copy link
Member Author

Choose a reason for hiding this comment

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

Since I didn't want to null-annotate two copies of basically the same function.

@lidavidm lidavidm merged commit 1e1a576 into apache:main Feb 9, 2024
10 checks passed
@lidavidm lidavidm deleted the gh624 branch February 9, 2024 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

java: add Checker Framework
2 participants