Skip to content

Change the behavior to return the correct format for day and year intervals. #40

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

Merged
escobargabriel merged 5 commits into from
May 5, 2022
Merged

Change the behavior to return the correct format for day and year intervals. #40

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
55fc9a1
Change the behavior to return the correct format for day and year int…
escobargabriel May 4, 2022
133d809
Test if holder.isSet indicates the value is null and return null dire…
escobargabriel May 5, 2022
6347675
Change the getString and getObject methods
escobargabriel May 5, 2022
bd16af2
Include a test passing a Joda object
escobargabriel May 5, 2022
7f09eb0
Remove checkstyle problem
escobargabriel May 5, 2022
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
87 changes: 49 additions & 38 deletions ...pache/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcIntervalVectorAccessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,34 +19,27 @@

import static org.apache.arrow.driver.jdbc.utils.IntervalStringUtils.formatIntervalDay;
import static org.apache.arrow.driver.jdbc.utils.IntervalStringUtils.formatIntervalYear;
import static org.joda.time.Period.parse;
import static org.apache.arrow.vector.util.DateUtility.yearsToMonths;

import java.sql.SQLException;
import java.time.Duration;
import java.time.Period;
import java.util.function.IntSupplier;

import org.apache.arrow.driver.jdbc.accessor.ArrowFlightJdbcAccessor;
import org.apache.arrow.driver.jdbc.accessor.ArrowFlightJdbcAccessorFactory;
import org.apache.arrow.vector.BaseFixedWidthVector;
import org.apache.arrow.vector.IntervalDayVector;
import org.apache.arrow.vector.IntervalYearVector;
import org.apache.arrow.vector.holders.NullableIntervalDayHolder;
import org.apache.arrow.vector.holders.NullableIntervalYearHolder;
import org.joda.time.Period;

/**
* Accessor for the Arrow type {@link IntervalDayVector}.
*/
public class ArrowFlightJdbcIntervalVectorAccessor extends ArrowFlightJdbcAccessor {

/**
* Functional interface used to unify Interval*Vector#getAsStringBuilder implementations.
*/
@FunctionalInterface
interface StringBuilderGetter {
StringBuilder get(int index);
}

private final BaseFixedWidthVector vector;
private final StringBuilderGetter stringBuilderGetter;
private final StringGetter stringGetter;
private final Class<?> objectClass;

/**
Expand All @@ -61,8 +54,18 @@ public ArrowFlightJdbcIntervalVectorAccessor(IntervalDayVector vector,
ArrowFlightJdbcAccessorFactory.WasNullConsumer setCursorWasNull) {
super(currentRowSupplier, setCursorWasNull);
this.vector = vector;
this.stringBuilderGetter = vector::getAsStringBuilder;
this.objectClass = Duration.class;
stringGetter = (index) -> {
final NullableIntervalDayHolder holder = new NullableIntervalDayHolder();
vector.get(index, holder);
if (holder.isSet == 0) {
return null;
} else {
final int days = holder.days;
final int millis = holder.milliseconds;
return formatIntervalDay(new Period().plusDays(days).plusMillis(millis));
}
};
objectClass = java.time.Duration.class;
}

/**
Expand All @@ -77,39 +80,47 @@ public ArrowFlightJdbcIntervalVectorAccessor(IntervalYearVector vector,
ArrowFlightJdbcAccessorFactory.WasNullConsumer setCursorWasNull) {
super(currentRowSupplier, setCursorWasNull);
this.vector = vector;
this.stringBuilderGetter = vector::getAsStringBuilder;
this.objectClass = Period.class;
stringGetter = (index) -> {
final NullableIntervalYearHolder holder = new NullableIntervalYearHolder();
vector.get(index, holder);
Copy link
Copy Markdown
Owner

@rafael-telles rafael-telles May 4, 2022

Choose a reason for hiding this comment

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

If holder indicates the value is null, we can return null directly

Copy link
Copy Markdown
Collaborator Author

@escobargabriel escobargabriel May 4, 2022

Choose a reason for hiding this comment

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

Done.

if (holder.isSet == 0) {
return null;
} else {
final int interval = holder.value;
final int years = (interval / yearsToMonths);
final int months = (interval % yearsToMonths);
return formatIntervalYear(new Period().plusYears(years).plusMonths(months));
}
};
objectClass = java.time.Period.class;
}

@Override
public Object getObject() {
Object object = this.vector.getObject(getCurrentRow());
this.wasNull = object == null;
this.wasNullConsumer.setWasNull(this.wasNull);

return object;
public Class<?> getObjectClass() {
return objectClass;
}

@Override
public Class<?> getObjectClass() {
return this.objectClass;
public String getString() throws SQLException {
String result = stringGetter.get(getCurrentRow());
wasNull = result == null;
wasNullConsumer.setWasNull(wasNull);
return result;
}

@Override
public String getString() throws SQLException {
Object object = getObject();
public Object getObject() {
Copy link
Copy Markdown
Owner

@rafael-telles rafael-telles May 4, 2022

Choose a reason for hiding this comment

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

Lets roll back getObject. It was more performant before

Object object = vector.getObject(getCurrentRow());
wasNull = object == null;
wasNullConsumer.setWasNull(wasNull);
return object;
}

this.wasNull = object == null;
this.wasNullConsumer.setWasNull(this.wasNull);
if (object == null) {
return null;
}
if (vector instanceof IntervalDayVector) {
return formatIntervalDay(parse(object.toString()));
} else if (vector instanceof IntervalYearVector) {
return formatIntervalYear(parse(object.toString()));
} else {
throw new SQLException("Invalid Interval vector instance");
}
/**
* Functional interface used to unify Interval*Vector#getAsStringBuilder implementations.
*/
@FunctionalInterface
interface StringGetter {
String get(int index);
}
}
10 changes: 10 additions & 0 deletions ...e/arrow/driver/jdbc/accessor/impl/calendar/ArrowFlightJdbcIntervalVectorAccessorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,8 @@ public void testShouldGetIntervalYear( ) {

@Test
public void testShouldGetIntervalDay( ) {
Assert.assertEquals("-001 00:00:00.000", formatIntervalDay(parse("PT-24H")));
Assert.assertEquals("+001 00:00:00.000", formatIntervalDay(parse("PT+24H")));
Comment on lines +170 to +171
Copy link
Copy Markdown
Collaborator

@vfraga vfraga May 4, 2022

Choose a reason for hiding this comment

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

Let's create an org.joda.time.Period object here and use plusDays/plusMillis instead of parsing a String. And we should simulate larger differences like +/-1567 00:00:00.000.

Copy link
Copy Markdown
Collaborator

@vfraga vfraga May 4, 2022

Choose a reason for hiding this comment

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

Also, where are the IntervalYear tests?

Assert.assertEquals("-000 01:00:00.000", formatIntervalDay(parse("PT-1H")));
Assert.assertEquals("-000 01:00:00.001", formatIntervalDay(parse("PT-1H-0M-00.001S")));
Assert.assertEquals("-000 01:01:01.000", formatIntervalDay(parse("PT-1H-1M-1S")));
Expand All @@ -185,6 +187,14 @@ public void testShouldGetIntervalDay( ) {
Assert.assertEquals("+000 22:22:22.222", formatIntervalDay(parse("PT+22H22M22.222S")));
}

@Test
public void testIntervalDayWithJodaPeriodObject() {
Assert.assertEquals("+1567 00:00:00.000",
formatIntervalDay(new org.joda.time.Period().plusDays(1567)));
Assert.assertEquals("-1567 00:00:00.000",
formatIntervalDay(new org.joda.time.Period().minusDays(1567)));
}

@Test
public void testShouldGetStringReturnCorrectString() throws Exception {
accessorIterator.assertAccessorGetter(vector, ArrowFlightJdbcIntervalVectorAccessor::getString,
Expand Down