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

Task/tests type conversions #26

Merged
merged 86 commits into from
Nov 4, 2018

Conversation

jpizagno
Copy link
Contributor

@morazow
Feel free to comment on anything, and we can change it.
What are your thoughts ?

morazow and others added 30 commits September 5, 2018 17:14
Add initial build matrix for spark versions 2.1.0 and 2.3.1
If Spark analyzer tells that we only need some columns, then update the Exasol query string to
select only those required columns.
This is helpful in integration tests. Because sometimes, SparkContext ends the Spark application
which closes the main jdbc connection. But then re-uses that SparkContext again. In this case we
will be asking a closed connection for parallel sub connections.
The scalastyle settings in tests (both unit and integration) are relaxed. For example, it is a
warning to use null or magic numbers in tests.
TODO: Differentiate based on datatypes, otherwise we make everything as string in comparison and
`in` clauses. For example, `col = '123.5'` this should be `col = 123.5` depending on type.
For example, for equality check, `str = 'abc'` and `num = 4` (not num = '4').
A solution proposed on #1 by @beatbull.

This adds a prefix of dummy table name to all columns in a query.
Because the connector is going to be used in Spark environment and it comes with scala-library
already included.
Adds integration tests for predicate pushdowns
Closes #2
Initial infrastructure for doing artifact automated releases from travis
Closes exasol#3
So they do not work if you set them in the fork of repo?
Then from maven or sbt the library can be referenced as

`com.exasol` % `spark-connector` % `version`
@morazow
Copy link
Contributor

morazow commented Oct 31, 2018

Hello @jpizagno,

Great! Thanks for working on this! Going to review.

@3cham
Copy link
Contributor

3cham commented Nov 1, 2018

@jpizagno @jamespiz don't forget to squash the commits & config your correct github username ;)

@jpizagno jpizagno force-pushed the task/tests-type-conversions branch from 9732c0f to 7754caa Compare November 1, 2018 15:51
…dded. refactored names of columns and methods. changed IT according to pull-request review
@jpizagno jpizagno force-pushed the task/tests-type-conversions branch 2 times, most recently from 19d67c6 to 687dfcd Compare November 1, 2018 16:06
@jpizagno
Copy link
Contributor Author

jpizagno commented Nov 1, 2018

@morazow
I squashed the commits, and reset my username. There is now a merge conflict, which I do not have write permissions to fix. Can you resolve the conflicts? The Merge has no conflicts now, I fixed them. Let me know if you have more comments?

@morazow
Copy link
Contributor

morazow commented Nov 2, 2018

@jpizagno apart from the typo string all looks good. Please let me know once it is removed; then we can merge the pr.

@jpizagno
Copy link
Contributor Author

jpizagno commented Nov 3, 2018

@morazow
I removed the typo. my apologies for letting that slip through ... again. The unit-tests and Integration tests are green. Let me know if you have more comments or suggestions?

@morazow
Copy link
Contributor

morazow commented Nov 4, 2018

@jpizagno great! lgtm!
I am going to merge the pr soon. Thanks for working on this!

@morazow morazow merged commit b797b2f into exasol:master Nov 4, 2018
@jpizagno
Copy link
Contributor Author

jpizagno commented Nov 4, 2018

@morazow
Sorry about this, but during the squash my work-git and home-git account names were both included. Now it looks like there are too many authors (jamespiz=wrong and jpizagno=correct). I will not mix this up in the future.

You can fix this following this guide:
https://help.github.com/articles/changing-author-info/
OLD_EMAIL="james.pizagno@otto.de"
CORRECT_NAME="jpizagno"
CORRECT_EMAIL="jpizagno@gmail.com"

morazow added a commit that referenced this pull request Nov 5, 2018
@morazow
Copy link
Contributor

morazow commented Nov 5, 2018

Good morning @jpizagno ,

No worries! I have updated the commits.

All the Best

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.

5 participants