Data conversion: timing data types. Testing #7

bpintea · 2018-05-29T15:19:24Z

This PR continues the data conversion development item.

An application is allowed to specify what it wants written in the provided buffers, for each select column in a result set. The driver is then responsible to convert the data between what's been received from the data source (ES/SQL) to what buffer type the application bound to those columns.

In the potential conversion matrix, some conversions are explicitly unsupported (like float to timestamp). The driver reduces further the possibility set, by not supporting some data types (like GUID or interval types).

This PR adds conversions to: DATE, TIME and TIMESTAMP.
It also handles the interval types, by rejecting the conversion.

The PR also introduces Google Test as the testing framework; the four tests suites added relate to the above functionality.

the value stays the same, but this is the technical right way

ODBC excludes certain data conversions between SQL/DB tyeps and application C buffers. This commit adds a check done - and cached - at SQLFetch() time, that compares the types received in result set columns to types assigned by application to the bound buffers.

c/p err, used lenght buffer (`pcbCharAttr`) instead of character buffer (`pCharAttr`).

- add gtest to the build system; - replace environment vars with cmake options for external dependencies location (to eventually be fetched by cmake's ExternalProject); - add a few new params to the bulid batch to better support the testing framework, symbols available in the DLL and platform detection.

The SQL_API marker was copied by mistake as function attribute.

Only queries deals with JSON code, move UJArraySize() functions there.

ujson4c exports one symbols with __declspec. mask it in the generated .LIB and exported anonymously in the .DLL. This as alternative to patching the library.

Data conversions make use of the SQL data types. Make sure that the value reported by 'SYS TYPES' query corresponds to the name of the data type, which is actually used as data key (since more types with different names - and other various parameters - can share some SQL type).

this connection mode allows passing the ES/SQL data types to a connection, w/o actually to connect to an Elasticsearch server.

Export into the output library some functions that allow attaching a (faked) server answer. This allows for testing w/o an Elasticsearch server.

- implement all allowed conversions to SQL_C_TYPE_TIMESTAMP.

- Break the loop when running individual tests; - use less indentation on the help output.

- add the possible conversions to the respective time types

- also change the declaration of the values used for testing to minimize duplication

...to include the data type tested.

- these are currently following ES/SQL lack of support and offer no conversion support; - added another check for convertability to pick out these unsupported types (GUID, interval) once, at Fetch time, to allow for faster failure.

droberts195 · 2018-05-30T11:05:53Z

CMakeLists.txt

-				"'ODBC-Specification' export into local 'lib' dir.")
-	endif (IS_DIRECTORY ${CMAKE_SOURCE_DIR}/libs/ODBC-Specification)
-endif (IS_DIRECTORY $ENV{ODBC_PATH_SRC})
+# TODO: add this as a git submodule and/or as ExternalProject


I think a git submodule will require extra work in release manager. A git subtree would not.

Thanks. git subtree would indeed be a lighter option.

droberts195 · 2018-05-30T11:17:47Z

test/connected_dbc.cc

@@ -0,0 +1,140 @@
+


None of the test files have a copyright header.

Thanks. Will added them. I guess I'll add the old license header and change it along with the migration to the new license (otherwise I'd have to add the top license file already).

droberts195 · 2018-05-30T11:21:32Z

driver/util.h

@@ -34,6 +34,14 @@
 #include "sql.h"
 #include "sqlext.h"

+#ifndef NDEBUG


I'm not sure it's a good idea to couple whether assertions are enabled with whether the tests are being run. Even if it's only very rarely used I think it would be best to have the option to build a non-test library with assertions and also run the tests with assertions compiled out.

No, not really, you're right. I initially started looking into identifying/controlling in CMake the build type (Debug/Release etc) when using the MSVC generators, but that took a bit longer than I wanted and default to this solution. But I'll resume that effort now and adjust this.

decoupled them in f3f376e.

droberts195 · 2018-05-30T11:25:09Z

driver/util.h

@@ -34,6 +34,14 @@
 #include "sql.h"
 #include "sqlext.h"

+#ifndef NDEBUG
+/* export attribute for internal functions used for testing */
+#define TEST_API	__declspec(dllexport)


Do you ever have a need to include the headers that use TEST_API in a program that's using the driver DLL? If you do maybe you need some logic like we use in ML to define the macro to dllimport instead of dllexport in this case, for example see https://github.com/elastic/ml-cpp/blob/7e1528bbaa016a4bc0e97b4b3f005e84dc831b9b/include/maths/ImportExport.h#L23

Only the unit testing executables would make use of the TEST_API exports.
(In no other case would those functions be used: the Driver Manager that loads the driver and dispatches the API calls would not know how to use any other exported symbol, except the ODBC spec ones.)
AFAIU however, even in this case the dllimport would help (generate "more efficient code", though for testing only in this case), so it looks like it'd be a good idea to add it anyways?

added it with a1b0596.

droberts195 · 2018-05-30T11:28:31Z

driver/util.c

@@ -80,7 +100,10 @@ BOOL wstr2long(wstr_st *val, long *out)
 int ansi_w2c(const SQLWCHAR *src, char *dst, size_t chars)
 {
 	int i = 0;
-
+
+	if (chars < 1)


I think you said before that you were aiming to always use braces with if statements. I guess the coding style is up to you, but just checking if you do want to adhere to that policy.

No, that's true. Fixed it, thanks.

edsavage · 2018-05-31T11:04:30Z

test/CMakeLists.txt

+	string(REPLACE ".cxx" "" TBIN ${TSRC})
+	add_executable(${TBIN} ${TSRC} ${EXTRA_SRC})
+	set_target_properties(${TBIN} PROPERTIES COMPILE_FLAGS ${CMAKE_C_FLAGS})
+	target_link_libraries(${TBIN} ${GTEST_LD_PATH}/gtestd.lib)


You might want to consider providing a separate, common main routine and linking against that (or using the one provided by the gtest_main library) here instead. It will reduce the duplication of the common main routine in each of the test files and simplify the running of the entire test suite.

It could also be useful to generate test reports using the --gtest_output command line arg (or the corresponding environment variable). This could be useful when integrating with CI down the line...

You might want to consider linking against the gtest_main library here instead. It will reduce the duplication of the common main routine in each of the test files and simplify the running of the entire test suite.

Linked against the main lib, thanks. (I've actually started like that, but removed it for some reason and then just copy&pasted with the main in).

It could also be useful to generate test reports using the --gtest_output command line arg (or the corresponding environment variable).

I would leave this to be controlled by the presence of the env vars, mainly b/c I'm not sure how to manage the generated files. But if you had something specific in mind, sure, I'll add it in.
(FWIW, currently, the build script will generate a report, both when invoking the RUN_TESTS target (test arg), as well as when executing the individual tests itself (suites arg) ).

edsavage

LGTM.

I've made a couple of suggestion regarding the test suite.

- take the main() out of the tests, use google test lib provided for this.

- using the "previous" Elastic license for now.

The goal is be able to run the tests in both cases, when the asserts are in the code or out. This is now achieved by only defining TEST_API macro, when it's not already done. The Release and MinSizeRel build configs will define it, so the test functions won't be exported anymore (i.e. tests can't be run). Debug and RelWithDebInfo will not define it, so tests will be possible.

- besides the (arbitrary) minor and major numbers and the unicode/ansi indicators ('u'/'a'), the string now contains also the the commit/tag and what build config it was used: 'd'(ebug), 'r'(elease) etc.

- the build.bat will take a new parameter, type=T, where T can be one of Debug, Release, RelWithDebInfo or MinSizeRel

- as alternative to the dllexport one, when building the driver, used with test calls

droberts195

LGTM

Data conversion: timing data types. Testing

bpintea added 18 commits May 16, 2018 11:27

b/f: use the wide chars variants in switch

95b2242

the value stays the same, but this is the technical right way

b/f: fix destination buffer pointer as argument

dd23003

c/p err, used lenght buffer (`pcbCharAttr`) instead of character buffer (`pCharAttr`).

b/f: two internal functions now no longer exported

65bd930

The SQL_API marker was copied by mistake as function attribute.

move JSON specific code to proper module

831ba55

Only queries deals with JSON code, move UJArraySize() functions there.

mask unnecessary symbols exported by 3rdp library

eb55d46

ujson4c exports one symbols with __declspec. mask it in the generated .LIB and exported anonymously in the .DLL. This as alternative to patching the library.

add a 'testing' mode to connecting

cebd335

this connection mode allows passing the ES/SQL data types to a connection, w/o actually to connect to an Elasticsearch server.

export some key functions used by testing

a3d339b

Export into the output library some functions that allow attaching a (faked) server answer. This allows for testing w/o an Elasticsearch server.

data conversions to C timestamp type

8d50695

- implement all allowed conversions to SQL_C_TYPE_TIMESTAMP.

add the tests for conversion to SQL C timestamp

feff775

when running 'suites' abort on failure

7ce4b97

- Break the loop when running individual tests; - use less indentation on the help output.

add convertion to SQL_C_TYPE_DATE and _TIME

908bd33

- add the possible conversions to the respective time types

tests for _TIME and _DATE conversions

7e95add

add state checking loggic to connected dbc class

ebc307c

- also change the declaration of the values used for testing to minimize duplication

renamed timestamp tests file

ed939ad

...to include the data type tested.

add handling of interval C data types

7961311

- these are currently following ES/SQL lack of support and offer no conversion support; - added another check for convertability to pick out these unsupported types (GUID, interval) once, at Fetch time, to allow for faster failure.

bpintea requested review from droberts195 and edsavage May 29, 2018 15:19

bpintea mentioned this pull request May 29, 2018

TODOs #2

Closed

droberts195 reviewed May 30, 2018

View reviewed changes

edsavage reviewed May 31, 2018

View reviewed changes

style: brace single-line blocks

9a8de6d

edsavage reviewed May 31, 2018

View reviewed changes

bpintea added 5 commits May 31, 2018 14:28

use gtest_maind lib for the main

5e8d5c7

- take the main() out of the tests, use google test lib provided for this.

add the license header in all test files

b0a34b4

- using the "previous" Elastic license for now.

add build info to the version string

56409a4

- besides the (arbitrary) minor and major numbers and the unicode/ansi indicators ('u'/'a'), the string now contains also the the commit/tag and what build config it was used: 'd'(ebug), 'r'(elease) etc.

add ability to select the configuration to build

b5087f3

- the build.bat will take a new parameter, type=T, where T can be one of Debug, Release, RelWithDebInfo or MinSizeRel

bpintea added 2 commits June 1, 2018 17:57

add dllimport attribute to TEST_API

a1b0596

- as alternative to the dllexport one, when building the driver, used with test calls

added the missing license headers

286f200

droberts195 approved these changes Jun 4, 2018

View reviewed changes

bpintea merged commit 0c68261 into elastic:master Jun 4, 2018

bpintea deleted the feature/data_conversion branch June 4, 2018 10:19

bpintea added a commit that referenced this pull request Jun 4, 2018

Merge pull request #7 from bpintea/feature/data_conversion

cfa7eeb

Data conversion: timing data types. Testing

bpintea added >feature Applicable to PRs adding new functionality v6.5.0 >test PRs targeting exclusively the testing framework labels May 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Data conversion: timing data types. Testing #7

Data conversion: timing data types. Testing #7

bpintea commented May 29, 2018 •

edited

Loading

droberts195 May 30, 2018

bpintea May 31, 2018

droberts195 May 30, 2018

bpintea May 31, 2018

droberts195 May 30, 2018

bpintea May 31, 2018 •

edited

Loading

bpintea Jun 1, 2018

droberts195 May 30, 2018

bpintea May 31, 2018 •

edited

Loading

bpintea Jun 1, 2018

droberts195 May 30, 2018

bpintea May 31, 2018

edsavage May 31, 2018 •

edited

Loading

bpintea May 31, 2018

edsavage left a comment

droberts195 left a comment

Data conversion: timing data types. Testing #7

Data conversion: timing data types. Testing #7

Conversation

bpintea commented May 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpintea May 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpintea May 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edsavage May 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edsavage left a comment

Choose a reason for hiding this comment

droberts195 left a comment

Choose a reason for hiding this comment

bpintea commented May 29, 2018 •

edited

Loading

bpintea May 31, 2018 •

edited

Loading

bpintea May 31, 2018 •

edited

Loading

edsavage May 31, 2018 •

edited

Loading