Skip to content

Commit ccaac9a

Browse files
Kaustav-SarkarKaustav-Sarkar
authored
GH-125: Allow null timestamp holder sans timezone (#941)
## Description Fixes an `IllegalArgumentException` in `TimeStamp*TZVector.set/setSafe` when unsetting values using a holder with a `null` timezone. The validation logic now correctly ignores the timezone check when `holder.isSet <= 0`, allowing default-constructed holders to be used for unsetting values as expected. Comprehensive tests added for all timestamp precisions (Micro, Milli, Nano, Sec) to verify the fix and ensure the existing workaround (setting explicit timezone) remains supported. Closes #125 .
1 parent 44c49ba commit ccaac9a

File tree

5 files changed

+217
-20
lines changed

5 files changed

+217
-20
lines changed

‎vector/src/main/java/org/apache/arrow/vector/TimeStampMicroTZVector.java‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -155,12 +155,13 @@ public void set(int index, NullableTimeStampMicroTZHolder holder)
155155
throws IllegalArgumentException {
156156
if (holder.isSet < 0) {
157157
throw new IllegalArgumentException();
158-
} else if (!this.timeZone.equals(holder.timezone)) {
159-
throw new IllegalArgumentException(
160-
String.format(
161-
"holder.timezone: %s not equal to vector timezone: %s",
162-
holder.timezone, this.timeZone));
163158
} else if (holder.isSet > 0) {
159+
if (!this.timeZone.equals(holder.timezone)) {
160+
throw new IllegalArgumentException(
161+
String.format(
162+
"holder.timezone: %s not equal to vector timezone: %s",
163+
holder.timezone, this.timeZone));
164+
}
164165
BitVectorHelper.setBit(validityBuffer, index);
165166
setValue(index, holder.value);
166167
} else {

‎vector/src/main/java/org/apache/arrow/vector/TimeStampMilliTZVector.java‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -155,12 +155,13 @@ public void set(int index, NullableTimeStampMilliTZHolder holder)
155155
throws IllegalArgumentException {
156156
if (holder.isSet < 0) {
157157
throw new IllegalArgumentException();
158-
} else if (!this.timeZone.equals(holder.timezone)) {
159-
throw new IllegalArgumentException(
160-
String.format(
161-
"holder.timezone: %s not equal to vector timezone: %s",
162-
holder.timezone, this.timeZone));
163158
} else if (holder.isSet > 0) {
159+
if (!this.timeZone.equals(holder.timezone)) {
160+
throw new IllegalArgumentException(
161+
String.format(
162+
"holder.timezone: %s not equal to vector timezone: %s",
163+
holder.timezone, this.timeZone));
164+
}
164165
BitVectorHelper.setBit(validityBuffer, index);
165166
setValue(index, holder.value);
166167
} else {

‎vector/src/main/java/org/apache/arrow/vector/TimeStampNanoTZVector.java‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -154,12 +154,13 @@ public Long getObject(int index) {
154154
public void set(int index, NullableTimeStampNanoTZHolder holder) throws IllegalArgumentException {
155155
if (holder.isSet < 0) {
156156
throw new IllegalArgumentException();
157-
} else if (!this.timeZone.equals(holder.timezone)) {
158-
throw new IllegalArgumentException(
159-
String.format(
160-
"holder.timezone: %s not equal to vector timezone: %s",
161-
holder.timezone, this.timeZone));
162157
} else if (holder.isSet > 0) {
158+
if (!this.timeZone.equals(holder.timezone)) {
159+
throw new IllegalArgumentException(
160+
String.format(
161+
"holder.timezone: %s not equal to vector timezone: %s",
162+
holder.timezone, this.timeZone));
163+
}
163164
BitVectorHelper.setBit(validityBuffer, index);
164165
setValue(index, holder.value);
165166
} else {

‎vector/src/main/java/org/apache/arrow/vector/TimeStampSecTZVector.java‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -150,12 +150,13 @@ public Long getObject(int index) {
150150
public void set(int index, NullableTimeStampSecTZHolder holder) throws IllegalArgumentException {
151151
if (holder.isSet < 0) {
152152
throw new IllegalArgumentException();
153-
} else if (!this.timeZone.equals(holder.timezone)) {
154-
throw new IllegalArgumentException(
155-
String.format(
156-
"holder.timezone: %s not equal to vector timezone: %s",
157-
holder.timezone, this.timeZone));
158153
} else if (holder.isSet > 0) {
154+
if (!this.timeZone.equals(holder.timezone)) {
155+
throw new IllegalArgumentException(
156+
String.format(
157+
"holder.timezone: %s not equal to vector timezone: %s",
158+
holder.timezone, this.timeZone));
159+
}
159160
BitVectorHelper.setBit(validityBuffer, index);
160161
setValue(index, holder.value);
161162
} else {

‎vector/src/test/java/org/apache/arrow/vector/TestValueVector.java‎

Lines changed: 193 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,10 @@
5757
import org.apache.arrow.vector.complex.impl.UnionListViewWriter;
5858
import org.apache.arrow.vector.complex.impl.UnionListWriter;
5959
import org.apache.arrow.vector.holders.NullableIntHolder;
60+
import org.apache.arrow.vector.holders.NullableTimeStampMicroTZHolder;
61+
import org.apache.arrow.vector.holders.NullableTimeStampMilliTZHolder;
62+
import org.apache.arrow.vector.holders.NullableTimeStampNanoTZHolder;
63+
import org.apache.arrow.vector.holders.NullableTimeStampSecTZHolder;
6064
import org.apache.arrow.vector.holders.NullableUInt4Holder;
6165
import org.apache.arrow.vector.holders.NullableVarBinaryHolder;
6266
import org.apache.arrow.vector.holders.NullableVarCharHolder;
@@ -2567,6 +2571,195 @@ public void testSetNullableVarCharHolderSafe() {
25672571
}
25682572
}
25692573

2574+
@Test
2575+
public void testTimeStampTZVectorSetSafeUnset() {
2576+
// reproduction of https://github.com/apache/arrow/issues/45084
2577+
try (TimeStampMicroTZVector vector = new TimeStampMicroTZVector("vector", allocator, "UTC")) {
2578+
vector.allocateNew();
2579+
// Set a valid value
2580+
NullableTimeStampMicroTZHolder validHolder = new NullableTimeStampMicroTZHolder();
2581+
validHolder.isSet = 1;
2582+
validHolder.value = 1000L;
2583+
validHolder.timezone = "UTC";
2584+
vector.setSafe(0, validHolder);
2585+
2586+
assertEquals(1000L, vector.get(0));
2587+
2588+
// Unset the value using a holder with default (null) timezone
2589+
// The bug used to throw IllegalArgumentException because holder.timezone (null) !=
2590+
// vector.timezone ("UTC")
2591+
// The correct behaviour is to not throw an exception and to unset the value.
2592+
NullableTimeStampMicroTZHolder unsetHolder = new NullableTimeStampMicroTZHolder();
2593+
unsetHolder.isSet = 0;
2594+
vector.setSafe(0, unsetHolder);
2595+
2596+
assertNull(vector.getObject(0));
2597+
}
2598+
}
2599+
2600+
@Test
2601+
public void testTimeStampMilliTZVectorSetSafeUnset() {
2602+
// reproduction of https://github.com/apache/arrow/issues/45084
2603+
try (TimeStampMilliTZVector vector = new TimeStampMilliTZVector("vector", allocator, "UTC")) {
2604+
vector.allocateNew();
2605+
2606+
NullableTimeStampMilliTZHolder validHolder = new NullableTimeStampMilliTZHolder();
2607+
validHolder.isSet = 1;
2608+
validHolder.value = 1000L;
2609+
validHolder.timezone = "UTC";
2610+
vector.setSafe(0, validHolder);
2611+
2612+
assertEquals(1000L, vector.get(0));
2613+
2614+
NullableTimeStampMilliTZHolder unsetHolder = new NullableTimeStampMilliTZHolder();
2615+
unsetHolder.isSet = 0;
2616+
vector.setSafe(0, unsetHolder);
2617+
2618+
assertNull(vector.getObject(0));
2619+
}
2620+
}
2621+
2622+
@Test
2623+
public void testTimeStampNanoTZVectorSetSafeUnset() {
2624+
// reproduction of https://github.com/apache/arrow/issues/45084
2625+
try (TimeStampNanoTZVector vector = new TimeStampNanoTZVector("vector", allocator, "UTC")) {
2626+
vector.allocateNew();
2627+
2628+
NullableTimeStampNanoTZHolder validHolder = new NullableTimeStampNanoTZHolder();
2629+
validHolder.isSet = 1;
2630+
validHolder.value = 1000L;
2631+
validHolder.timezone = "UTC";
2632+
vector.setSafe(0, validHolder);
2633+
2634+
assertEquals(1000L, vector.get(0));
2635+
2636+
NullableTimeStampNanoTZHolder unsetHolder = new NullableTimeStampNanoTZHolder();
2637+
unsetHolder.isSet = 0;
2638+
vector.setSafe(0, unsetHolder);
2639+
2640+
assertNull(vector.getObject(0));
2641+
}
2642+
}
2643+
2644+
@Test
2645+
public void testTimeStampSecTZVectorSetSafeUnset() {
2646+
// reproduction of https://github.com/apache/arrow/issues/45084
2647+
try (TimeStampSecTZVector vector = new TimeStampSecTZVector("vector", allocator, "UTC")) {
2648+
vector.allocateNew();
2649+
2650+
NullableTimeStampSecTZHolder validHolder = new NullableTimeStampSecTZHolder();
2651+
validHolder.isSet = 1;
2652+
validHolder.value = 1000L;
2653+
validHolder.timezone = "UTC";
2654+
vector.setSafe(0, validHolder);
2655+
2656+
assertEquals(1000L, vector.get(0));
2657+
2658+
NullableTimeStampSecTZHolder unsetHolder = new NullableTimeStampSecTZHolder();
2659+
unsetHolder.isSet = 0;
2660+
vector.setSafe(0, unsetHolder);
2661+
2662+
assertNull(vector.getObject(0));
2663+
}
2664+
}
2665+
2666+
@Test
2667+
public void testTimeStampMicroTZVectorSetSafeUnsetExplicitTimezone() {
2668+
// Test to ensure fix added for https://github.com/apache/arrow/issues/45084 does not break
2669+
// workaround.
2670+
try (TimeStampMicroTZVector vector = new TimeStampMicroTZVector("vector", allocator, "UTC")) {
2671+
vector.allocateNew();
2672+
2673+
NullableTimeStampMicroTZHolder validHolder = new NullableTimeStampMicroTZHolder();
2674+
validHolder.isSet = 1;
2675+
validHolder.value = 1000L;
2676+
validHolder.timezone = "UTC";
2677+
vector.setSafe(0, validHolder);
2678+
2679+
assertEquals(1000L, vector.get(0));
2680+
2681+
NullableTimeStampMicroTZHolder unsetHolder = new NullableTimeStampMicroTZHolder();
2682+
unsetHolder.isSet = 0;
2683+
unsetHolder.timezone = "UTC";
2684+
2685+
vector.setSafe(0, unsetHolder);
2686+
2687+
assertNull(vector.getObject(0));
2688+
}
2689+
}
2690+
2691+
@Test
2692+
public void testTimeStampMilliTZVectorSetSafeUnsetExplicitTimezone() {
2693+
// Test to ensure fix added for https://github.com/apache/arrow/issues/45084 does not break
2694+
// workaround.
2695+
try (TimeStampMilliTZVector vector = new TimeStampMilliTZVector("vector", allocator, "UTC")) {
2696+
vector.allocateNew();
2697+
2698+
NullableTimeStampMilliTZHolder validHolder = new NullableTimeStampMilliTZHolder();
2699+
validHolder.isSet = 1;
2700+
validHolder.value = 1000L;
2701+
validHolder.timezone = "UTC";
2702+
vector.setSafe(0, validHolder);
2703+
2704+
assertEquals(1000L, vector.get(0));
2705+
2706+
NullableTimeStampMilliTZHolder unsetHolder = new NullableTimeStampMilliTZHolder();
2707+
unsetHolder.isSet = 0;
2708+
unsetHolder.timezone = "UTC";
2709+
vector.setSafe(0, unsetHolder);
2710+
2711+
assertNull(vector.getObject(0));
2712+
}
2713+
}
2714+
2715+
@Test
2716+
public void testTimeStampNanoTZVectorSetSafeUnsetExplicitTimezone() {
2717+
// Test to ensure fix added for https://github.com/apache/arrow/issues/45084 does not break
2718+
// workaround.
2719+
try (TimeStampNanoTZVector vector = new TimeStampNanoTZVector("vector", allocator, "UTC")) {
2720+
vector.allocateNew();
2721+
2722+
NullableTimeStampNanoTZHolder validHolder = new NullableTimeStampNanoTZHolder();
2723+
validHolder.isSet = 1;
2724+
validHolder.value = 1000L;
2725+
validHolder.timezone = "UTC";
2726+
vector.setSafe(0, validHolder);
2727+
2728+
assertEquals(1000L, vector.get(0));
2729+
2730+
NullableTimeStampNanoTZHolder unsetHolder = new NullableTimeStampNanoTZHolder();
2731+
unsetHolder.isSet = 0;
2732+
unsetHolder.timezone = "UTC";
2733+
vector.setSafe(0, unsetHolder);
2734+
2735+
assertNull(vector.getObject(0));
2736+
}
2737+
}
2738+
2739+
@Test
2740+
public void testTimeStampSecTZVectorSetSafeUnsetExplicitTimezone() {
2741+
// Test to ensure fix added for https://github.com/apache/arrow/issues/45084 does not break
2742+
// workaround.
2743+
try (TimeStampSecTZVector vector = new TimeStampSecTZVector("vector", allocator, "UTC")) {
2744+
vector.allocateNew();
2745+
2746+
NullableTimeStampSecTZHolder validHolder = new NullableTimeStampSecTZHolder();
2747+
validHolder.isSet = 1;
2748+
validHolder.value = 1000L;
2749+
validHolder.timezone = "UTC";
2750+
vector.setSafe(0, validHolder);
2751+
2752+
assertEquals(1000L, vector.get(0));
2753+
2754+
NullableTimeStampSecTZHolder unsetHolder = new NullableTimeStampSecTZHolder();
2755+
unsetHolder.isSet = 0;
2756+
unsetHolder.timezone = "UTC";
2757+
vector.setSafe(0, unsetHolder);
2758+
2759+
assertNull(vector.getObject(0));
2760+
}
2761+
}
2762+
25702763
@Test
25712764
public void testSetNullableVarBinaryHolder() {
25722765
try (VarBinaryVector vector = new VarBinaryVector("", allocator)) {

0 commit comments

Comments
 (0)