[GitHub] drill issue #602: Improve Drill C++ connector
Github user laurentgo commented on the issue: https://github.com/apache/drill/pull/602 Branch updated with only commits associated with a JIRA (everything else has been merged into commit for DRILL-4420) --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill issue #602: Improve Drill C++ connector
Github user laurentgo commented on the issue: https://github.com/apache/drill/pull/602 yes, let me clean my branch one last time by squashing the small commits with no JIRA. For the windows build, I already added instructions regarding the need for PowerShell on the system, and on how to get/install cppunit, can you have a look? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill issue #602: Improve Drill C++ connector
Github user parthchandra commented on the issue: https://github.com/apache/drill/pull/602 +1. Everything looks good. I'm assuming that you will squash the commits which don't have an associated Jira. Also, if you can add any notes needed to the Windows build instructions, that would be useful. Thanks for patiently fixing all the issues! --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill issue #602: Improve Drill C++ connector
Github user laurentgo commented on the issue: https://github.com/apache/drill/pull/602 sounds good, I think all of the comments I received have been addressed. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill issue #602: Improve Drill C++ connector
Github user parthchandra commented on the issue: https://github.com/apache/drill/pull/602 No need for many PRs. Let's just squash this branch into commits for the individual JIRA's I 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill issue #602: Improve Drill C++ connector
Github user laurentgo commented on the issue: https://github.com/apache/drill/pull/602 @parthchandra do you want individual pull requests for each issue, or are you okay having this branch squashed and merged? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill issue #602: Improve Drill C++ connector
Github user laurentgo commented on the issue: https://github.com/apache/drill/pull/602 New version of the branch with a fix for the version detection (also added the option to provide the version from the commandline if needed) + disable secure iterator check which causes all these warning messages with protobuf. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill issue #602: Improve Drill C++ connector
Github user parthchandra commented on the issue:
https://github.com/apache/drill/pull/602
@laurentgo What version of zookeeper libs are you using on Windows?
FindZookeeper is failing for me because my version (3.4.6), has a
zookeeper.lib file, not zookeeper_d.lib.
Also, I get the following with cmake -
> CMake Error at CMakeLists.txt:43 (string):
> string sub-command REGEX, mode REPLACE needs at least 6 arguments total
to
> command.
>
Tracked it down to select-xml not finding the drill version in the pom file
> PS C:\Users\Administrator\Documents\drill\contrib> Select-Xml -Namespace
@{'m'='http://maven.apache.org/POM/4.0.0'} -XPath
'//m:project/m:version/text()' -Path
C:\Users\Administrator\Documents\drill\contrib\native\client\..\..\pom.xml |
foreach {$_.Node.Value}
> PS C:\Users\Administrator\Documents\drill\contrib>
> PS C:\Users\Administrator\Documents\drill\contrib>
>
This, however, works -
> PS C:\Users\Administrator\Documents\drill\contrib> Select-Xml -Namespace
@{'m'='http://maven.apache.org/POM/4.0.0'} -XPath
'//m:project/m:version/text()' -Path
C:\Users\Administrator\Documents\drill\contrib\native\client\..\..\pom.xml |
foreach {$_.Node.Value}
> PS C:\Users\Administrator\Documents\drill\contrib>
> PS C:\Users\Administrator\Documents\drill\contrib> Select-Xml -Namespace
@{'m'='http://maven.apache.org/POM/4.0.0'} -XPath
'//m:project/m:parent/m:version/text()' -Path
C:\Users\Administrator\Documents\drill\contrib\native\client\..\..\pom.xml |
foreach {$_.Node.V
> alue}
> 1.9.0-SNAPSHOT
> PS C:\Users\Administrator\Documents\drill\contrib>
>
Also, the Debug build has a new warning it would be nice if this can be
removed, not a showstopper though) -
```
4>C:\Program Files (x86)\Microsoft Visual Studio
12.0\VC\include\xutility(2132): warning C4996: 'std::_Copy_impl': Function call
with parameters that may be unsafe - this call relies on the caller to check
that the passed values are correct. To disable this warning, use
-D_SCL_SECURE_NO_WARNINGS. See documentation on how to use Visual C++ 'Checked
Iterators'
4> C:\Program Files (x86)\Microsoft Visual Studio
12.0\VC\include\xutility(2113) : see declaration of 'std::_Copy_impl'
4>
C:\Users\Administrator\Documents\drill\contrib\native\client\src\clientlib\drillClientImpl.cpp(561)
: see reference to function template instantiation '_OutIt
std::copy(_InIt,_InIt,_OutIt)'
being compiled
4> with
4> [
4>
_OutIt=google::protobuf::internal::RepeatedPtrFieldBackInsertIterator
4> ,
_InIt=std::_Vector_const_iterator ]
```
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill issue #602: Improve Drill C++ connector
Github user laurentgo commented on the issue: https://github.com/apache/drill/pull/602 Just uploaded a new version of the branch @parthchandra: on windows, I'm using PowerShell to extract the project information from the pom.xml instead of a call to maven (which I assume is present on linux and mac). The call to sed is replaced with the cmake string command. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill issue #602: Improve Drill C++ connector
Github user parthchandra commented on the issue: https://github.com/apache/drill/pull/602 I just put the FindCppUnit hack in my branch here: https://github.com/parthchandra/drill/tree/DRILL-4420 I hit another issue (bit of a show stopper) with windows. In order to get on with the build, I had hardcoded the drill version info that is generated by the call to maven (I didn't have maven installed). After installing maven and getting it to work correctly, I found that the Cmake script did not execute correctly because Drill_src seems to come back with forward slashes which Windows interprets as command options. Fixing it by hand and trying to run maven on the command line fails with the error: Cannot ruin program echo. Windows cmd has an 'echo' which does something entirely unexpected. Also there is no 'sed'. I don't know a solution to this yet. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill issue #602: Improve Drill C++ connector
Github user laurentgo commented on the issue: https://github.com/apache/drill/pull/602 Thanks @parthchandra for the feedback 1) I'm still struggling having a working Windows dev environment. Regarding CppUnit, it seems the LibreOffice folks are using the version hosted on FDo (https://freedesktop.org/wiki/Software/cppunit/). It seems the source provides instructions on how to build on windows at https://cgit.freedesktop.org/libreoffice/cppunit/tree/INSTALL-WIN32.txt?id=cppunit-1.13.2 Do you mind posting changes to FindCppUnit so I can add them to the patch? (unless I find a way to build on windows meanwhile) 2) To get the version from the pom, I would need at least a xml parser, like libxml2, so I could write a script like that one: ``` xmllint --shell pom.xml << EOT > setns maven=http://maven.apache.org/POM/4.0.0 > cd //maven:project/maven:version/text() > write version > bye > EOT ``` but I guess having xmllint2 installed on a windows box is a similar burden. I'm open to suggestions (maybe @superbstreak have some ideas?), otherwise I'll add a valid Java/Maven environment as a prerequisite 4) It looks like llvm and xcode have a different set of warnings, I'll check those and fix them. More generally, do you want to keep this pull request as one change (and merge it as one commit), or do you prefer I open separate pull requests for each DRILL jira I addressed? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill issue #602: Improve Drill C++ connector
Github user parthchandra commented on the issue: https://github.com/apache/drill/pull/602 @laurentgo These changes are looking good. I just have the following observations - 1) The build on Windows was a problem for me. I found CppUnit here (http://dev-www.libreoffice.org/src/cppunit-1.13.2.tar.gz). There is a Windows solutions file that will build on VS 2013 However FindCppUnit needs to be updated to find CppUnit on Windows as there is no standard location the build will install to. 2) The cmake script now requires maven which in turn requires the JDK!. Is it possible to generate the product version from the root pom? This would be useful as the version is automatically changed at the time of the release. Either way you should update the windows build instructions on where to get CppUnit. Also, include instructions that the build requires JDK and Maven as this was not previously required. 3) I hacked findCppUnit and that found the libs and header files, but the unit tests failed to compile. I did not spend too much time on this though. 4) On MacOs, I upgraded my ZK libs to 3.4.7 (I built from source) and the build goes thru fine. There are some new warnings which should be removed - `/Users/pchandra/work/drill/contrib/native/client/src/clientlib/drillClientImpl.hpp:364:4: warning: field 'm_bIsDirectConnection' will be initialized after field 'm_bIsConnected' [-Wreorder] m_bIsDirectConnection(false), ^ /Users/pchandra/work/drill/contrib/native/client/src/clientlib/drillClientImpl.hpp:583:34: warning: field 'm_bIsDirectConnection' will be initialized after field 'm_maxConcurrentConnections' [-Wreorder] PooledDrillClientImpl(): m_bIsDirectConnection(false), m_maxConcurrentConnections(DEFAULT_MAX_CONCURRENT_CONNECTIONS), m_lastConnection(-1), m_pError(NULL), m_quer... ^ /Users/pchandra/work/drill/contrib/native/client/src/clientlib/drillClientImpl.hpp:583:64: warning: field 'm_maxConcurrentConnections' will be initialized after field 'm_lastConnection' [-Wreorder] PooledDrillClientImpl(): m_bIsDirectConnection(false), m_maxConcurrentConnections(DEFAULT_MAX_CONCURRENT_CONNECTIONS), m_lastConnection(-1), m_pError(NULL), m_quer... ^ /Users/pchandra/work/drill/contrib/native/client/src/clientlib/drillClientImpl.hpp:583:150: warning: field 'm_pError' will be initialized after field 'm_queriesExecuted' [-Wreorder] PooledDrillClientImpl(): m_bIsDirectConnection(false), m_maxConcurrentConnections(DEFAULT_MAX_CONCURRENT_CONNECTIONS), m_lastConnection(-1), m_pError(NULL), m_quer... ` Other than that, things are really good. Even though it made reviewing the code much harder, thank you for refactoring and cleaning up the code. BTW, I also built and tested the build on linux, windows and macos. I also ran a bunch of queries and cancels thru valgrind, and everything was nice and clean. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill issue #602: Improve Drill C++ connector
Github user superbstreak commented on the issue: https://github.com/apache/drill/pull/602 **Mac Environment:** OS: OSX 10.8.5 COMPILER=xcode5_1 **Libraries:** Zookeeper: 3.4.6 (patched) boost: 1.57.0 protobuf: 2.5.0rc1 **For libc++:** .../contrib/native/client/CMakeLists.txt: - Add: set(CMAKE_CXX_FLAGS "-stdlib=libc++") before if(CMAKE_COMPILER_IS_GNUCXX) - Change to set(CMAKE_EXE_LINKER_FLAGS "-stdlib=libc++ -lrt -lpthread") - Change to set(CMAKE_CXX_FLAGS "-fPIC -stdlib=libc++") **And the minimal code changes required for zookeeper to work:** #ifndef htonll <- Add this int64_t htonll(int64_t v); #endif < Add this --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill issue #602: Improve Drill C++ connector
Github user parthchandra commented on the issue: https://github.com/apache/drill/pull/602 OK. The Mac issue I see is caused by MacOS - https://issues.apache.org/jira/browse/ZOOKEEPER-2049 Just to confirm, what version of MacOS and Zookeeper did you build with? Should we upgrade the client to use Zookeeper 3.4.7? --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill issue #602: Improve Drill C++ connector
Github user laurentgo commented on the issue: https://github.com/apache/drill/pull/602 My build environment on Mac is OS X 10.11.6 (El Capitan) and Zookeeper 3.4.8 (brew package). I don't think the client needs to be updated if people are using an older version of OS X --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill issue #602: Improve Drill C++ connector
Github user parthchandra commented on the issue: https://github.com/apache/drill/pull/602 Let's discuss this offline. My goal would be to review (most changes look OK to me; but I want to take a more detailed look at c10e652 and a931903), build on Mac, Linux and Windows. I was also planning to run valgrind on Linux where I can build successfully. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill issue #602: Improve Drill C++ connector
Github user laurentgo commented on the issue: https://github.com/apache/drill/pull/602 I have built it successfully on Mac and Linux (centos 5). On Mac, I used homebrew to install cppunit locally on Mac, but I didn't try to build it from source. You can send me your output, and I will see if I can reproduce it on my laptop too. As of windows, unfortunately I was not able to create a working build environment with boost/protobuf/zookeeper, so I didn't even try to get/build cppunit on it. If you have good instructions on how to create a working build environment on windows for the Drill connector, I'd love to try it. That said, some people reported that my branch is compiling on windows, although I didn't ask if they were running the CppUnit tests. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill issue #602: Improve Drill C++ connector
Github user parthchandra commented on the issue: https://github.com/apache/drill/pull/602 @laurentgo I have been trying, unsuccessfully, to build on Windows and Mac. Is there a cppunit build available for Windows? I am unable to get past that and my effort to build cppunit from source has so far not been successful (something in my autotools setup). I can share the Mac build output if you want to take a look. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
