search

twitter / scalding

A Scala API for Cascading
http://twitter.com/scalding
Apache License 2.0
3.48k stars 703 forks source link

Adds support for blob, tinyint and timestamp to TypedJDBCSource #1948

Closed tlazaro closed 2 years ago

tlazaro commented 2 years ago

Trying to do some cleanup and have all Twitter internal changes pushed to open source. Please advise on whether this change makes sense as-is and whether there could be consequences for other users.

Will try to add unit tests.

tlazaro commented 2 years ago

Just found we had #1915 with this work already.

johnynek commented 2 years ago

is there any reason why the previous one was not merged @navinvishy ?

Looks like it just got lost somehow...

navinvishy commented 2 years ago

is there any reason why the previous one was not merged @navinvishy ?

Looks like it just got lost somehow...

Yeah, my bad - I think this just got lost. fwiw we've had these changes at Twitter for a while now.

tlazaro commented 2 years ago

Updated PR with unit tests from #1915.

I think we were hesitant because of some comments around TINYINT(1) being a BOOLEAN and DATETIME (this one was removed the patch) and both out of topic from the PR.

This code in this PR is what we have had in production for the past 2 years, happy to make changes if it makes sense but otherwise we are satisfied with these changes.

tlazaro commented 2 years ago

@johnynek do you think we can land this?

tlazaro commented 2 years ago

sorry for the latency.

I think the complete opposite is the truth, you have been incredibly responsive and helping out so much in such short notice. To the point I worry we are taking too much of your time.