Skip to content

GH-125: Fix Timestamp unset bug #941

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.

Sign up for GitHub

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

Open
Kaustav-Sarkar wants to merge 1 commit into
base: main
Choose a base branch
from
+197 −4
Open

GH-125: Fix Timestamp unset bug #941

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion vector/src/main/java/org/apache/arrow/vector/TimeStampMicroTZVector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ public void set(int index, NullableTimeStampMicroTZHolder holder)
throws IllegalArgumentException {
if (holder.isSet < 0) {
throw new IllegalArgumentException();
} else if (!this.timeZone.equals(holder.timezone)) {
} else if (holder.isSet > 0 && !this.timeZone.equals(holder.timezone)) {
throw new IllegalArgumentException(
String.format(
"holder.timezone: %s not equal to vector timezone: %s",
Expand Down
2 changes: 1 addition & 1 deletion vector/src/main/java/org/apache/arrow/vector/TimeStampMilliTZVector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ public void set(int index, NullableTimeStampMilliTZHolder holder)
throws IllegalArgumentException {
if (holder.isSet < 0) {
throw new IllegalArgumentException();
} else if (!this.timeZone.equals(holder.timezone)) {
} else if (holder.isSet > 0 && !this.timeZone.equals(holder.timezone)) {
throw new IllegalArgumentException(
String.format(
"holder.timezone: %s not equal to vector timezone: %s",
Expand Down
2 changes: 1 addition & 1 deletion vector/src/main/java/org/apache/arrow/vector/TimeStampNanoTZVector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ public Long getObject(int index) {
public void set(int index, NullableTimeStampNanoTZHolder holder) throws IllegalArgumentException {
if (holder.isSet < 0) {
throw new IllegalArgumentException();
} else if (!this.timeZone.equals(holder.timezone)) {
} else if (holder.isSet > 0 && !this.timeZone.equals(holder.timezone)) {
throw new IllegalArgumentException(
String.format(
"holder.timezone: %s not equal to vector timezone: %s",
Expand Down
2 changes: 1 addition & 1 deletion vector/src/main/java/org/apache/arrow/vector/TimeStampSecTZVector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ public Long getObject(int index) {
public void set(int index, NullableTimeStampSecTZHolder holder) throws IllegalArgumentException {
if (holder.isSet < 0) {
throw new IllegalArgumentException();
} else if (!this.timeZone.equals(holder.timezone)) {
} else if (holder.isSet > 0 && !this.timeZone.equals(holder.timezone)) {
throw new IllegalArgumentException(
String.format(
"holder.timezone: %s not equal to vector timezone: %s",
Expand Down
193 changes: 193 additions & 0 deletions vector/src/test/java/org/apache/arrow/vector/TestValueVector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,10 @@
import org.apache.arrow.vector.complex.impl.UnionListViewWriter;
import org.apache.arrow.vector.complex.impl.UnionListWriter;
import org.apache.arrow.vector.holders.NullableIntHolder;
import org.apache.arrow.vector.holders.NullableTimeStampMicroTZHolder;
import org.apache.arrow.vector.holders.NullableTimeStampMilliTZHolder;
import org.apache.arrow.vector.holders.NullableTimeStampNanoTZHolder;
import org.apache.arrow.vector.holders.NullableTimeStampSecTZHolder;
import org.apache.arrow.vector.holders.NullableUInt4Holder;
import org.apache.arrow.vector.holders.NullableVarBinaryHolder;
import org.apache.arrow.vector.holders.NullableVarCharHolder;
Expand Down Expand Up @@ -2567,6 +2571,195 @@ public void testSetNullableVarCharHolderSafe() {
}
}

@Test
public void testTimeStampTZVectorSetSafeUnset() {
// reproduction of https://github.com/apache/arrow/issues/45084
try (TimeStampMicroTZVector vector = new TimeStampMicroTZVector("vector", allocator, "UTC")) {
vector.allocateNew();
// Set a valid value
NullableTimeStampMicroTZHolder validHolder = new NullableTimeStampMicroTZHolder();
validHolder.isSet = 1;
validHolder.value = 1000L;
validHolder.timezone = "UTC";
vector.setSafe(0, validHolder);

assertEquals(1000L, vector.get(0));

// Unset the value using a holder with default (null) timezone
// The bug used to throw IllegalArgumentException because holder.timezone (null) !=
// vector.timezone ("UTC")
// The correct behaviour is to not throw an exception and to unset the value.
NullableTimeStampMicroTZHolder unsetHolder = new NullableTimeStampMicroTZHolder();
unsetHolder.isSet = 0;
vector.setSafe(0, unsetHolder);

assertNull(vector.getObject(0));
}
}

@Test
public void testTimeStampMilliTZVectorSetSafeUnset() {
// reproduction of https://github.com/apache/arrow/issues/45084
try (TimeStampMilliTZVector vector = new TimeStampMilliTZVector("vector", allocator, "UTC")) {
vector.allocateNew();

NullableTimeStampMilliTZHolder validHolder = new NullableTimeStampMilliTZHolder();
validHolder.isSet = 1;
validHolder.value = 1000L;
validHolder.timezone = "UTC";
vector.setSafe(0, validHolder);

assertEquals(1000L, vector.get(0));

NullableTimeStampMilliTZHolder unsetHolder = new NullableTimeStampMilliTZHolder();
unsetHolder.isSet = 0;
vector.setSafe(0, unsetHolder);

assertNull(vector.getObject(0));
}
}

@Test
public void testTimeStampNanoTZVectorSetSafeUnset() {
// reproduction of https://github.com/apache/arrow/issues/45084
try (TimeStampNanoTZVector vector = new TimeStampNanoTZVector("vector", allocator, "UTC")) {
vector.allocateNew();

NullableTimeStampNanoTZHolder validHolder = new NullableTimeStampNanoTZHolder();
validHolder.isSet = 1;
validHolder.value = 1000L;
validHolder.timezone = "UTC";
vector.setSafe(0, validHolder);

assertEquals(1000L, vector.get(0));

NullableTimeStampNanoTZHolder unsetHolder = new NullableTimeStampNanoTZHolder();
unsetHolder.isSet = 0;
vector.setSafe(0, unsetHolder);

assertNull(vector.getObject(0));
}
}

@Test
public void testTimeStampSecTZVectorSetSafeUnset() {
// reproduction of https://github.com/apache/arrow/issues/45084
try (TimeStampSecTZVector vector = new TimeStampSecTZVector("vector", allocator, "UTC")) {
vector.allocateNew();

NullableTimeStampSecTZHolder validHolder = new NullableTimeStampSecTZHolder();
validHolder.isSet = 1;
validHolder.value = 1000L;
validHolder.timezone = "UTC";
vector.setSafe(0, validHolder);

assertEquals(1000L, vector.get(0));

NullableTimeStampSecTZHolder unsetHolder = new NullableTimeStampSecTZHolder();
unsetHolder.isSet = 0;
vector.setSafe(0, unsetHolder);

assertNull(vector.getObject(0));
}
}

@Test
public void testTimeStampMicroTZVectorSetSafeUnsetExplicitTimezone() {
// Test to ensure fix added for https://github.com/apache/arrow/issues/45084 does not break
// workaround.
try (TimeStampMicroTZVector vector = new TimeStampMicroTZVector("vector", allocator, "UTC")) {
vector.allocateNew();

NullableTimeStampMicroTZHolder validHolder = new NullableTimeStampMicroTZHolder();
validHolder.isSet = 1;
validHolder.value = 1000L;
validHolder.timezone = "UTC";
vector.setSafe(0, validHolder);

assertEquals(1000L, vector.get(0));

NullableTimeStampMicroTZHolder unsetHolder = new NullableTimeStampMicroTZHolder();
unsetHolder.isSet = 0;
unsetHolder.timezone = "UTC";

vector.setSafe(0, unsetHolder);

assertNull(vector.getObject(0));
}
}

@Test
public void testTimeStampMilliTZVectorSetSafeUnsetExplicitTimezone() {
// Test to ensure fix added for https://github.com/apache/arrow/issues/45084 does not break
// workaround.
try (TimeStampMilliTZVector vector = new TimeStampMilliTZVector("vector", allocator, "UTC")) {
vector.allocateNew();

NullableTimeStampMilliTZHolder validHolder = new NullableTimeStampMilliTZHolder();
validHolder.isSet = 1;
validHolder.value = 1000L;
validHolder.timezone = "UTC";
vector.setSafe(0, validHolder);

assertEquals(1000L, vector.get(0));

NullableTimeStampMilliTZHolder unsetHolder = new NullableTimeStampMilliTZHolder();
unsetHolder.isSet = 0;
unsetHolder.timezone = "UTC";
vector.setSafe(0, unsetHolder);

assertNull(vector.getObject(0));
}
}

@Test
public void testTimeStampNanoTZVectorSetSafeUnsetExplicitTimezone() {
// Test to ensure fix added for https://github.com/apache/arrow/issues/45084 does not break
// workaround.
try (TimeStampNanoTZVector vector = new TimeStampNanoTZVector("vector", allocator, "UTC")) {
vector.allocateNew();

NullableTimeStampNanoTZHolder validHolder = new NullableTimeStampNanoTZHolder();
validHolder.isSet = 1;
validHolder.value = 1000L;
validHolder.timezone = "UTC";
vector.setSafe(0, validHolder);

assertEquals(1000L, vector.get(0));

NullableTimeStampNanoTZHolder unsetHolder = new NullableTimeStampNanoTZHolder();
unsetHolder.isSet = 0;
unsetHolder.timezone = "UTC";
vector.setSafe(0, unsetHolder);

assertNull(vector.getObject(0));
}
}

@Test
public void testTimeStampSecTZVectorSetSafeUnsetExplicitTimezone() {
// Test to ensure fix added for https://github.com/apache/arrow/issues/45084 does not break
// workaround.
try (TimeStampSecTZVector vector = new TimeStampSecTZVector("vector", allocator, "UTC")) {
vector.allocateNew();

NullableTimeStampSecTZHolder validHolder = new NullableTimeStampSecTZHolder();
validHolder.isSet = 1;
validHolder.value = 1000L;
validHolder.timezone = "UTC";
vector.setSafe(0, validHolder);

assertEquals(1000L, vector.get(0));

NullableTimeStampSecTZHolder unsetHolder = new NullableTimeStampSecTZHolder();
unsetHolder.isSet = 0;
unsetHolder.timezone = "UTC";
vector.setSafe(0, unsetHolder);

assertNull(vector.getObject(0));
}
}

@Test
public void testSetNullableVarBinaryHolder() {
try (VarBinaryVector vector = new VarBinaryVector("", allocator)) {
Expand Down
Loading