Skip to content
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.

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

Feature/db ignore unknown columns refactoring #1284

Merged

Conversation

Kornel
Copy link
Contributor

@Kornel Kornel commented Jan 10, 2016

Hi,

I see you've @mbastian fixed the issue I've stumbled upon in #1272 - when the column is not known, gephi database import threw an NPE.

I did a different fix - first, it's always good practice to compare the constant to the field, not the field to the constant, then no null check is necessary. E.g. if (NodeProperty.ID.equals(foo)) instead of if (foo.equals(NodeProperty.ID)).

That's the change I did first, but then I also refactored the code a bit - I think it's redundant to iterate over the column names for each result set iteration. Why not fetch the id column metadata first, and then use it?

Thats my suggestion here, please let me know what you think.

Cheers,
Kornel

@Kornel
Copy link
Contributor Author

Kornel commented Jan 31, 2016

Hey,
@mbastian any thought on this PR?

K

@eduramiba eduramiba self-assigned this Oct 9, 2016
@eduramiba eduramiba added this to the 0.9.2 milestone Oct 9, 2016
@eduramiba
Copy link
Member

Thank you for the pull request, we will review and merge this as soon as possible.

@Kornel
Copy link
Contributor Author

Kornel commented Oct 9, 2016

No problem, glad I can help.
On Sun, 9 Oct 2016 at 12:52, Eduardo Ramos [email protected] wrote:

Thank you for the pull request, we will review and merge this as soon as
possible.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#1284 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAmW6eYusB1QsYSiU09ka6S0cMLE0bD1ks5qyMcIgaJpZM4HB7WF
.

@eduramiba
Copy link
Member

Merged, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants