DRILL-4091: Support for additional gis operations in gis contrib module#1201
Closed
ChrisSandison wants to merge 1 commit intoapache:masterfrom
Closed
DRILL-4091: Support for additional gis operations in gis contrib module#1201ChrisSandison wants to merge 1 commit intoapache:masterfrom
ChrisSandison wants to merge 1 commit intoapache:masterfrom
Conversation
Contributor
|
@cgivre could you review this? |
cgivre
requested changes
Apr 10, 2018
Contributor
cgivre
left a comment
cgivre
left a comment
There was a problem hiding this comment.
@kkhatua Sure.
@ChrisSandison It looks generally good. My initial comment/request is could you add a sentence in a comment for each function describing what the function does.
| import org.apache.drill.exec.expr.holders.VarBinaryHolder; | ||
|
|
||
| import io.netty.buffer.DrillBuf; | ||
|
|
Contributor
There was a problem hiding this comment.
Can you please include a sentence describing what this function does? Thanks!
| import org.apache.drill.exec.expr.holders.VarBinaryHolder; | ||
|
|
||
| import io.netty.buffer.DrillBuf; | ||
|
|
Contributor
There was a problem hiding this comment.
Please include a sentence describing the functionality of this UDF. Thanks!
| import org.apache.drill.exec.expr.holders.VarBinaryHolder; | ||
|
|
||
| import io.netty.buffer.DrillBuf; | ||
|
|
Contributor
There was a problem hiding this comment.
Please include a sentence describing the functionality of the UDF
| import org.apache.drill.exec.expr.holders.VarBinaryHolder; | ||
|
|
||
| import io.netty.buffer.DrillBuf; | ||
|
|
Contributor
There was a problem hiding this comment.
For all UDFs... I'd appreciate it if you could include a comment (one sentence will do and can be cut/pasted from PostGIS or wherever, describing what the function does.
ChrisSandison
force-pushed
the
DRILL-4091
branch
2 times, most recently
from
2f5166e to
599b150
Compare
Author
|
@cgivre added comments |
Contributor
|
@ChrisSandison This isn't a review comment, but @k255 was working on a format plugin for ESRI shape files. Would you want to take a look and see if we could get that in as well? I could see that being VERY useful once we get all this GIS functionality in. |
Author
|
@cgivre I could look into that. Is it on a separate ticket/PR? |
Contributor
|
If you want to include it on this PR that would be great, but I’m fine with including it in a separate PR as well. I want to get this functionality into Drill asap, so it probably would be quicker to do a separate PR.
… On Apr 10, 2018, at 10:32, Chris Sandison ***@***.***> wrote:
@cgivre <https://github.com/cgivre> do you mean including that in this PR?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#1201 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFQfvmYpl5h9CH5cLHPn7aweR0KUhbrTks5tnMJkgaJpZM4TFMOI>.
|
Author
ChrisSandison
force-pushed
the
DRILL-4091
branch
from
599b150 to
a1f3936
Compare
Contributor
|
@ChrisSandison Done. Here's a link to the JIRA https://issues.apache.org/jira/browse/DRILL-6319 |
Contributor
|
LGTM +1 One thing, which I suspect may be a problem with my environment, not this PR, but when I try to build the module with the tests, I get the following errors: |
Author
|
@cgivre I had some issues between this and a previous branch I was working on, but I fixed it by upgrade the base image that I was building it on. I ended up building it on maven:3.5.3-jdk-8 |
Contributor
|
In that case, LGTM +1 |
Contributor
|
@arina-ielchiieva Should I do the commit or would you like to do that. |
Member
|
@cgivre this PR won't pass Trevis checks. It has incorrect license headers, missing spaces ( |
Contributor
|
Thanks @arina-ielchiieva @ChrisSandison Can you please:
And we'll get it committed! Thanks! |
ChrisSandison
force-pushed
the
DRILL-4091
branch
2 times, most recently
from
01e75fe to
0df7914
Compare
Author
|
@cgivre updated and squashed |
ChrisSandison
force-pushed
the
DRILL-4091
branch
2 times, most recently
from
fd01a07 to
02cba80
Compare
Contributor
|
@ChrisSandison, Could you please remove these, squash the commits and I think we're done. |
ChrisSandison
force-pushed
the
DRILL-4091
branch
2 times, most recently
from
87055f7 to
39b9974
Compare
Contributor
|
+1 |
Contributor
|
@ChrisSandison |
Contributor
|
It's committed into master.
…On Sat, Jun 2, 2018 at 4:46 AM, Chris Sandison ***@***.***> wrote:
@parthchandra <https://github.com/parthchandra> @cgivre
<https://github.com/cgivre> why has this been closed?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1201 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AGOgcDFWkgQ38FXHvrsm5UbVtd1gLaNmks5t4nslgaJpZM4TFMOI>
.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This based off of PR #258. I have attempted to address the comments from the PR left by @cgivre
Some additional notes:
STUnionAggregate.java, there was a comment mentioning null handling, but I'm unable to find any null handling support when defining interfaces for aggregate functions.TestGeometryFunctions.java. The failing tests were attempting to test non-point geometry on functions that require it. While returningDouble.NaNseems appropriate for the situation, I found two issues with testing this:DefaultSequelHandler.transform()casts the results of theFloat8HoldertoBigIntegerwhich does not have a representation forNaN. As a result, this is returning aUserExceptiongiving a system error, which would be the expected behaviour. However, I can't seem to test for this exception given the test builder that is used.