Skip to content

Conversation

@gsvic
Copy link
Contributor

@gsvic gsvic commented Nov 2, 2020

No description provided.

@gsvic gsvic force-pushed the victorgiannakouris/null_support branch from a69c219 to c34a1a9 Compare November 2, 2020 15:12
@joe-maley joe-maley self-requested a review November 2, 2020 16:00
@joe-maley
Copy link

@gsvic I will take a look at this once the core PR is merged, hopefully next week.

@gsvic gsvic force-pushed the victorgiannakouris/null_support branch 3 times, most recently from dae5d6e to 0e8cc06 Compare November 12, 2020 16:10
@gsvic gsvic requested a review from Shelnutt2 November 12, 2020 16:11
Copy link

@joe-maley joe-maley left a comment

Choose a reason for hiding this comment

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

LGTM, two minor comments:

@@ -1,5 +1,5 @@
TILEDB_GIT_REPOSITORY=https://github.com/TileDB-Inc/TileDB
TILEDB_GIT_TAG=2.1.2
TILEDB_GIT_TAG=dev

Choose a reason for hiding this comment

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

Should we wait until we release 2.2 with null support?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree. I changed the tag to dev just for the CI. So let's wait for the 2.2 release.

public void testAttributeSetNullable() throws Exception {
try (Context ctx = new Context();
Attribute a = new Attribute(ctx, "a2", Datatype.TILEDB_FLOAT32)) {
a.setNullable((short) 0);

Choose a reason for hiding this comment

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

there appears to be an indentation issue here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that this is ok. If you look at it, the Attribute a = .. is inside the try parentheses (try (Context ctx = ... Attribute a = ...) similarly to an if () {} block. So that's why the a.setNullable (line 116) is positioned two tabs (\t) after the try (line 114).

What do you think?

@gsvic gsvic force-pushed the victorgiannakouris/null_support branch 2 times, most recently from c8efcd6 to 49286a6 Compare November 17, 2020 11:31
@gsvic gsvic force-pushed the victorgiannakouris/null_support branch from 49286a6 to 2eba444 Compare November 17, 2020 13:52
@gsvic gsvic requested a review from joe-maley November 17, 2020 14:13
@gsvic gsvic force-pushed the victorgiannakouris/null_support branch from 9e4359e to 2297da8 Compare January 27, 2021 10:06
}
}

public int setNullable(short nullable) throws TileDBError {
Copy link
Member

Choose a reason for hiding this comment

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

Should this be a bool?

@gsvic gsvic requested a review from Shelnutt2 January 27, 2021 19:27
@gsvic gsvic merged commit 521ac3a into master Jan 27, 2021
@gsvic gsvic deleted the victorgiannakouris/null_support branch January 27, 2021 20:04
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.

4 participants