Skip to content

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

Merged
escobargabriel merged 5 commits intoflight-jdbc-driverfrom
fix-day-and-year-interval-format
May 5, 2022
Merged

Change the behavior to return the correct format for day and year intervals.#40
escobargabriel merged 5 commits intoflight-jdbc-driverfrom
fix-day-and-year-interval-format

Conversation

@escobargabriel
Copy link
Collaborator

Changes were made to return the correct values for the intervals.

@jcralmeida jcralmeida self-requested a review May 4, 2022 17:17
stringGetter = (index) -> {
final NullableIntervalDayHolder holder = new NullableIntervalDayHolder();
vector.get(index, holder);
final int days = holder.days;
Copy link
Owner

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
Collaborator Author

Choose a reason for hiding this comment

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

Done.

return object;
stringGetter = (index) -> {
final NullableIntervalYearHolder holder = new NullableIntervalYearHolder();
vector.get(index, holder);
Copy link
Owner

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
Collaborator Author

Choose a reason for hiding this comment

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

Done.

this.wasNull = object == null;
this.wasNullConsumer.setWasNull(this.wasNull);
if (object == null) {
if (vector.isNull(getCurrentRow())) {
Copy link
Owner

Choose a reason for hiding this comment

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

If we make the previous change I suggested, we can call stringGetter and compare it to null instead of calling vector.isNull

}

@Override
public Object getObject() {
Copy link
Owner

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

if (object == null) {
if (vector.isNull(getCurrentRow())) {
wasNull = true;
wasNullConsumer.setWasNull(true);
Copy link
Owner

Choose a reason for hiding this comment

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

We should set wasNull and call setWasNull always, otherwise it never goes back to false

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

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
Collaborator

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?

@vfraga vfraga self-requested a review May 5, 2022 17:15
@vfraga
Copy link
Collaborator

vfraga commented May 5, 2022

Just ensure it builds.

@escobargabriel escobargabriel merged commit 7f1eeec into flight-jdbc-driver May 5, 2022
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