Skip to content

[CALCITE-2939] NPE when executeBatch array type.#94

Merged
F21 merged 2 commits intoapache:masterfrom
bakea:calcite-2939
Apr 23, 2019
Merged

[CALCITE-2939] NPE when executeBatch array type.#94
F21 merged 2 commits intoapache:masterfrom
bakea:calcite-2939

Conversation

@bakea
Copy link
Contributor

@bakea bakea commented Apr 10, 2019

No description provided.

}
if (componentRep == null && list.size() > 0) {
componentRep = ((TypedValue) list.get(0)).type;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The document already say that the componentType in TypedValue is a non-null value [1], the only way is that when the nested parse in [2], which is a null we passed in initiatively. For null i think it is reasonable cause we do not know what the value component type yet. The code you patched is reasonable if we finally got the list element type.

[1]

/** Non-null for ARRAYs, the type of the values stored in the ARRAY. Null for all other cases. */

[2]
copy.add(serialToJdbc(componentRep, null, o, calendar));

}
}
if (componentRep == null && list.size() > 0) {
componentRep = ((TypedValue) list.get(0)).type;
Copy link
Contributor

Choose a reason for hiding this comment

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

If we still can not got the type, we should throw an exception, can you please add a check 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 so.

@F21
Copy link
Member

F21 commented Apr 23, 2019

@danny0405 Can you look at the updated changes?

Copy link
Contributor

@danny0405 danny0405 left a comment

Choose a reason for hiding this comment

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

+1, LGTM

@F21 F21 merged commit 8482516 into apache:master Apr 23, 2019
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.

3 participants