You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@tajo.apache.org by eminency <gi...@git.apache.org> on 2016/05/17 03:47:32 UTC
[GitHub] tajo pull request: TAJO-2156: Create GeoIP functions taking variou...
GitHub user eminency opened a pull request:
https://github.com/apache/tajo/pull/1027
TAJO-2156: Create GeoIP functions taking various types instead of INET4 type
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/eminency/tajo geoip_funcs
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/tajo/pull/1027.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1027
----
commit 300f78964e475f1ba728d6a614379cbafef09ce1
Author: Jongyoung Park <em...@gmail.com>
Date: 2016-05-16T07:39:09Z
new geoip functions are added
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo issue #1027: TAJO-2156: Create GeoIP functions taking various types ins...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on the issue:
https://github.com/apache/tajo/pull/1027
+1 ship it!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-2156: Create GeoIP functions taking variou...
Posted by eminency <gi...@git.apache.org>.
Github user eminency commented on the pull request:
https://github.com/apache/tajo/pull/1027#issuecomment-219926775
Hi, @jinossy
Other geoip functions(old ones) have not had a test.
I think it is because of license problem - GeoIP library is set as 'provided' in pom.xml, so test code doesn't work.
Is that wrong?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-2156: Create GeoIP functions taking variou...
Posted by jinossy <gi...@git.apache.org>.
Github user jinossy commented on the pull request:
https://github.com/apache/tajo/pull/1027#issuecomment-219928681
The Initial code did not add a tests. because geoip is GPL.
Now, geoip function is breaking changes. we should add a tests.
GeoIP2 is Apache 2.0 license. so we can add the module and tests in tajo binary
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request #1027: TAJO-2156: Create GeoIP functions taking various ty...
Posted by eminency <gi...@git.apache.org>.
Github user eminency closed the pull request at:
https://github.com/apache/tajo/pull/1027
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-2156: Create GeoIP functions taking variou...
Posted by jinossy <gi...@git.apache.org>.
Github user jinossy commented on the pull request:
https://github.com/apache/tajo/pull/1027#issuecomment-219903294
This changes should be added tests.
If you change GeoIP to GeoIP2, it would be better for tests.
please see the lincese and csv database for testing
http://maxmind.github.io/GeoIP2-java
https://dev.maxmind.com/geoip/geoip2/geoip2-city-country-csv-databases
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo issue #1027: TAJO-2156: Create GeoIP functions taking various types ins...
Posted by jinossy <gi...@git.apache.org>.
Github user jinossy commented on the issue:
https://github.com/apache/tajo/pull/1027
+1 LGTM
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-2156: Create GeoIP functions taking variou...
Posted by eminency <gi...@git.apache.org>.
Github user eminency commented on the pull request:
https://github.com/apache/tajo/pull/1027#issuecomment-222081661
Separated issues and refined code.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-2156: Create GeoIP functions taking variou...
Posted by jinossy <gi...@git.apache.org>.
Github user jinossy commented on the pull request:
https://github.com/apache/tajo/pull/1027#issuecomment-220500912
@eminency
Geoip2 seems to need to add a mock DataReader.
If we add a geoip2 function by new jira issue, It would be better. What do you think?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo issue #1027: TAJO-2156: Create GeoIP functions taking various types ins...
Posted by eminency <gi...@git.apache.org>.
Github user eminency commented on the issue:
https://github.com/apache/tajo/pull/1027
Thanks for the review.
I'll commit.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo issue #1027: TAJO-2156: Create GeoIP functions taking various types ins...
Posted by jihoonson <gi...@git.apache.org>.
Github user jihoonson commented on the issue:
https://github.com/apache/tajo/pull/1027
This patch looks good to me.
@jinossy do you have any other comments?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-2156: Create GeoIP functions taking variou...
Posted by eminency <gi...@git.apache.org>.
Github user eminency commented on the pull request:
https://github.com/apache/tajo/pull/1027#issuecomment-220504281
Good suggestion. I will separate it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] tajo pull request: TAJO-2156: Create GeoIP functions taking variou...
Posted by eminency <gi...@git.apache.org>.
Github user eminency commented on the pull request:
https://github.com/apache/tajo/pull/1027#issuecomment-219928860
Oh, I got it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---