Fix: consider interval's precision. Allow non-aligned period values as interval encoding (#148)

bpintea · bpintea · commit c0ace37c7ea1 · 2019-05-02T11:59:07.000+02:00
* add the precision value to the interval types The interval types with a seconds component have the seconds precision as a property. With this commit the driver will read the Datetime(/Timestamp) scale and use that as precision for the interval with seconds component data type property, attached to the .precision field of the records. * fix: use IRD as precision src. instead of ARD When handling values received from ES/SQL, use the IRD descriptor instead of the ARD, when reading data characteristics (like the precision). Also, change the integration tests that were setting ARD fields - like precision - on string conversions. (This is wrong since the strings don't have a precision property, so the driver should not - and now will not - take this property into account.) * fix: decouple ES period format from interval type ES/SQL transmits interval values in a ISO8601 period value. An interval-to-period 1:1 mapping is possible, however ES/SQL doesn't adhere to this mapping. For instance: - instead of `P1DT1H` for `INTERVAL '1 1' DAY TO HOUR`, ES/SQL will send `PT25H` (which in a 1:1 mapping would be `INTERVAL '25' HOUR` literal, equivalent, but different from the original); - `INTERVAL '61:59' MINUTE TO SECOND` is encoded as `PT1H1M59S` instead of `PT61M59S`; - `INTERVAL '61' SECOND` is transmitted as `PT1M1S` vs. `PT61S`. Because of this, the driver will now detect the mapping misalignment, convert any interval in the "day/to/second" range to seconds and re-calculate the values for the corresponding interval members from that. (cherry picked from commit 0366a74)
diff --git a/driver/connect.c b/driver/connect.c
@@ -1913,7 +1913,7 @@ static void *copy_types_rows(esodbc_dbc_st *dbc, estype_row_st *type_row,
 	SQLWCHAR *pos;
 	int c;
 	SQLULEN i;
-	SQLSMALLINT sql_type;
+	SQLSMALLINT sql_type, sec_prec;
 
 	/* pointer to start position where the strings will be copied in */
 	pos = (SQLWCHAR *)&types[rows_fetched];
@@ -1946,6 +1946,7 @@ static void *copy_types_rows(esodbc_dbc_st *dbc, estype_row_st *type_row,
 		} \
 	} while (0)
 
+	sec_prec = -1;
 	for (i = 0; i < rows_fetched; i ++) {
 		/* copy row data */
 		ES_TYPES_COPY_WSTR(type_name);
@@ -2028,6 +2029,20 @@ static void *copy_types_rows(esodbc_dbc_st *dbc, estype_row_st *type_row,
 			&types[i].sql_datetime_sub);
 
 		set_display_size(types + i);
+
+		/* There's no explicit coverage in the docs on how to communicate the
+		 * interval seconds precision, so will use the scales in
+		 * SQLGetTypeInfo() for that, just like for Timestamp type;
+		 * furthermore, will use Timestamp's value, since it's the same
+		 * seconds precision across all ES/SQL types with seconds component. */
+		if (types[i].data_type == SQL_TYPE_TIMESTAMP) {
+			assert(sec_prec < 0);
+			sec_prec = types[i].maximum_scale;
+		} else if (types[i].meta_type == METATYPE_INTERVAL_WSEC) {
+			assert(0 < sec_prec);
+			types[i].maximum_scale = sec_prec;
+			types[i].minimum_scale = types[i].maximum_scale;
+		}
 	}
 
 #undef ES_TYPES_COPY_INT
diff --git a/driver/convert.c b/driver/convert.c
@@ -1778,11 +1778,11 @@ static inline SQLRETURN adjust_to_precision(esodbc_rec_st *rec,
 	}
 }
 
-static SQLRETURN parse_iso8601_number(esodbc_rec_st *arec, wstr_st *wstr,
+static SQLRETURN parse_iso8601_number(esodbc_rec_st *rec, wstr_st *wstr,
 	SQLUINTEGER *uint, char *sign,
 	SQLUINTEGER *fraction, BOOL *has_fraction)
 {
-	esodbc_stmt_st *stmt = arec->desc->hdr.stmt;
+	esodbc_stmt_st *stmt = rec->desc->hdr.stmt;
 	char inc;
 	wstr_st nr;
 	int digits, fdigits;
@@ -1815,7 +1815,7 @@ static SQLRETURN parse_iso8601_number(esodbc_rec_st *arec, wstr_st *wstr,
 			ERRH(stmt, "fraction value too large (%llu).", ubint);
 			return SQL_ERROR;
 		} else {
-			ret = adjust_to_precision(arec, &ubint, fdigits);
+			ret = adjust_to_precision(rec, &ubint, fdigits);
 			assert(ubint < ULONG_MAX); /* due to previous sanity checks */
 			*fraction = (SQLUINTEGER)ubint;
 		}
@@ -1847,13 +1847,19 @@ static SQLRETURN parse_iso8601_number(esodbc_rec_st *arec, wstr_st *wstr,
 	return ret;
 }
 
-/* parse an ISO8601 period value
- * Elasticsearch'es implementation deviates slightly, hiding the day field:
- * `INTERVAL '1 1' DAY TO HOUR` -> `PT25H` instead of `P1DT1H`. */
-static SQLRETURN parse_interval_iso8601(esodbc_rec_st *arec,
+/* Parse an ISO8601 period value.
+ * Elasticsearch'es implementation deviates from the standard by, for example:
+ * - demoting the day field: `INTERVAL '1 1' DAY TO HOUR` ->
+ *   `PT25H` instead of `P1DT1H`; `INTERVAL '1' DAY` -> `PT24H` vs. `P1D`;
+ * - promoting the hour field: `INTERVAL '61:59' MINUTE TO SECOND` ->
+ *   `PT1H1M59S` instead of `PT61M59S`; `INTERVAL '60' MINUTE` -> `PT1H` vs.
+ *   `PT60M`.
+ * - promoting the minute field: `INTERVAL '61' SECOND` -> `PT1M1S` vs.
+ *   `PT61S` */
+static SQLRETURN parse_interval_iso8601(esodbc_rec_st *rec,
 	SQLSMALLINT ctype, wstr_st *wstr, SQL_INTERVAL_STRUCT *ivl)
 {
-	esodbc_stmt_st *stmt = arec->desc->hdr.stmt;
+	esodbc_stmt_st *stmt = rec->desc->hdr.stmt;
 	char sign;
 	SQLWCHAR *crr, *end;
 	wstr_st nr;
@@ -1877,7 +1883,9 @@ static SQLRETURN parse_interval_iso8601(esodbc_rec_st *arec,
 			(1 << SQL_IS_HOUR) | (1 << SQL_IS_MINUTE) | (1 << SQL_IS_SECOND),
 			(1 << SQL_IS_MINUTE) | (1 << SQL_IS_SECOND),
 	};
+	uint16_t type2bm_ivl; /* the type bit mask for the interval */
 	SQLRETURN ret;
+	SQLUINTEGER secs; /* ~ond~ */
 
 	/* Sets a bit in a bitmask corresponding to one interval field, given
 	 * `_ivl`, or errs if already set.
@@ -1910,6 +1918,15 @@ static SQLRETURN parse_interval_iso8601(esodbc_rec_st *arec,
 		DBGH(stmt, "field %d assigned value %lu.", _ivl_type, uint); \
 		state = saved; \
 	} while (0)
+	/* Safe ulong addition */
+#	define ULONG_SAFE_ADD(_to, _from) \
+	do { \
+		if (ULONG_MAX - (_from) < _to) { \
+			goto err_overflow; \
+		} else { \
+			_to += _from; \
+		} \
+	} while (0)
 
 	/* the interval type will be used as bitmask indexes */
 	assert(0 < SQL_IS_YEAR && SQL_IS_MINUTE_TO_SECOND < 16);
@@ -1954,7 +1971,7 @@ static SQLRETURN parse_interval_iso8601(esodbc_rec_st *arec,
 				}
 				nr.str = crr;
 				nr.cnt = end - crr;
-				ret = parse_iso8601_number(arec, &nr, &uint, &sign,
+				ret = parse_iso8601_number(rec, &nr, &uint, &sign,
 						&fraction, &has_fraction);
 				if (! SQL_SUCCEEDED(ret)) {
 					goto err_format;
@@ -2017,28 +2034,82 @@ static SQLRETURN parse_interval_iso8601(esodbc_rec_st *arec,
 
 	assert(0 < /*starts at 1*/ ivl->interval_type &&
 		ivl->interval_type < 8 * sizeof(type2bm)/sizeof(type2bm[0]));
-	/* reinstate the day field merged by ES in the hour field when:
-	 * - the day field hasn't been set;
-	 * - it is an interval with day component;
-	 * - the hour field overflows a day/24h  */
-	if (((1 << SQL_IS_DAY) & fields_bm) == 0 &&
-		(type2bm[ivl->interval_type - 1] & (1 << SQL_IS_DAY)) &&
-		24 <= ivl->intval.day_second.hour) {
-		ivl->intval.day_second.day = ivl->intval.day_second.hour / 24;
-		ivl->intval.day_second.hour = ivl->intval.day_second.hour % 24;
-		fields_bm |= 1 << SQL_IS_DAY;
+
+	type2bm_ivl = type2bm[ivl->interval_type - 1];
+	/* If the expression set fields not directly relevant to the interval AND
+	 * the interval is not of type year/-/month one (which does seem to
+	 * conform to the standard), rebalance the member values as expected by
+	 * the interval type. */
+	if ((type2bm_ivl != fields_bm) && ((type2bm_ivl &
+				((1 << SQL_CODE_YEAR) | (1 << SQL_CODE_MONTH))) == 0)) {
+		secs = ivl->intval.day_second.second;
+		ULONG_SAFE_ADD(secs, 60 * ivl->intval.day_second.minute); //...
+		ULONG_SAFE_ADD(secs, 3600 * ivl->intval.day_second.hour);
+		ULONG_SAFE_ADD(secs, 24 * 3600 * ivl->intval.day_second.day);
+		/* clear everything set, but reinstate any set fractions */
+		fields_bm = 0;
+		memset(&ivl->intval.day_second, 0, sizeof(ivl->intval.day_second));
+		ivl->intval.day_second.fraction = fraction;
+
+		if (type2bm_ivl & (1 << SQL_CODE_SECOND)) {
+			ivl->intval.day_second.second = secs;
+			fields_bm |= (1 << SQL_CODE_SECOND);
+		} else if (has_fraction) {
+			/* fraction val itself is truncated away due to precision
+			 * zero/null for intervals with no seconds component */
+			ERRH(stmt, "fraction in interval with no second component");
+			goto err_format;
+		}
+		if (type2bm_ivl & (1 << SQL_CODE_MINUTE)) {
+			if (ivl->intval.day_second.second) {
+				ivl->intval.day_second.minute =
+					ivl->intval.day_second.second / 60;
+				ivl->intval.day_second.second = secs % 60;
+			} else {
+				ivl->intval.day_second.minute = secs / 60;
+				assert(secs % 60 == 0);
+			}
+			fields_bm |= (1 << SQL_CODE_MINUTE);
+		}
+		if (type2bm_ivl & (1 << SQL_CODE_HOUR)) {
+			if (ivl->intval.day_second.minute) {
+				ivl->intval.day_second.hour =
+					ivl->intval.day_second.minute / 60;
+				ivl->intval.day_second.minute %= 60;
+			} else {
+				ivl->intval.day_second.hour = secs / 3600;
+				assert(secs % 3600 == 0);
+			}
+			fields_bm |= (1 << SQL_CODE_HOUR);
+		}
+		if (type2bm_ivl & (1 << SQL_CODE_DAY)) {
+			if (ivl->intval.day_second.hour) {
+				ivl->intval.day_second.day =
+					ivl->intval.day_second.hour / 24;
+				ivl->intval.day_second.hour %= 24;
+			} else {
+				ivl->intval.day_second.day = secs / (24 * 3600);
+				assert(secs % (24 * 3600) == 0);
+			}
+			fields_bm |= (1 << SQL_CODE_DAY);
+		}
 	}
 
 	/* Check that the ISO value has no fields set other than those allowed
 	 * for the advertised type. Since the year_month and day_second form a
 	 * union, this can't be done by checks against field values. */
-	if ((~type2bm[ivl->interval_type - 1]) & fields_bm) {
+	if (~type2bm_ivl & fields_bm) {
 		ERRH(stmt, "illegal fields (0x%hx) for interval type %hd (0x%hx).",
-			fields_bm, ctype, type2bm[ivl->interval_type - 1]);
+			fields_bm, ctype, type2bm_ivl);
 		goto err_format;
 	}
 
 	return ret;
+
+err_overflow:
+	ERRH(stmt, "integer overflow while normalizing ISO8601 format [%zu] `"
+		LWPDL "`.", wstr->cnt, LWSTR(wstr));
+	RET_HDIAGS(stmt, SQL_STATE_22015);
 err_parse:
 	ERRH(stmt, "unexpected current char `%c` in state %d.", *crr, state);
 err_format:
@@ -2048,6 +2119,7 @@ static SQLRETURN parse_interval_iso8601(esodbc_rec_st *arec,
 
 #	undef ASSIGN_FIELD
 #	undef SET_BITMASK_OR_ERR
+#	undef ULONG_SAFE_ADD
 }
 
 /* Parse one field of the value.
@@ -2095,6 +2167,9 @@ static SQLRETURN parse_interval_field(esodbc_rec_st *rec, SQLUINTEGER limit,
 	return SQL_SUCCESS;
 }
 
+/* Interval precision:
+ * https://docs.microsoft.com/en-us/sql/odbc/reference/appendixes/interval-data-type-precision
+ */
 static SQLRETURN parse_interval_second(esodbc_rec_st *rec, SQLUINTEGER limit,
 	wstr_st *wstr, SQL_INTERVAL_STRUCT *ivl)
 {
@@ -2695,8 +2770,6 @@ static size_t print_interval_iso8601(esodbc_rec_st *rec,
 		case SQL_IS_HOUR_TO_MINUTE:
 		case SQL_IS_HOUR_TO_SECOND:
 		case SQL_IS_MINUTE_TO_SECOND:
-			// TODO: compoound year to hour, ES/SQL-style?
-			// (see parse_interval_iso8601 note)
 			PRINT_FIELD(day_second.day, 'D', /* is time comp. */FALSE);
 			t_added = FALSE;
 			PRINT_FIELD(day_second.hour, 'H', /*is time*/TRUE);
@@ -2749,12 +2822,12 @@ static SQLRETURN interval_iso8601_to_sql(esodbc_rec_st *arec,
 
 	ivl_wstr.str = (SQLWCHAR *)wstr;
 	ivl_wstr.cnt = *chars_0 - 1;
-	ret = parse_interval_iso8601(arec, irec->es_type->data_type, &ivl_wstr,
+	ret = parse_interval_iso8601(irec, irec->es_type->data_type, &ivl_wstr,
 			&ivl);
 	if (! SQL_SUCCEEDED(ret)) {
 		return ret;
 	}
-	cnt = print_interval_sql(arec, &ivl, (SQLWCHAR *)lit);
+	cnt = print_interval_sql(irec, &ivl, (SQLWCHAR *)lit);
 	if (cnt <= 0) {
 		ERRH(stmt, "sql interval printing failed for ISO8601`" LWPDL "`.",
 			chars_0 - 1, wstr);
diff --git a/driver/queries.c b/driver/queries.c
@@ -159,6 +159,11 @@ static SQLRETURN attach_columns(esodbc_stmt_st *stmt, UJObject columns)
 			rec->type = rec->es_type->sql_data_type;
 			rec->datetime_interval_code = rec->es_type->sql_datetime_sub;
 			rec->meta_type = rec->es_type->meta_type;
+			/* set INTERVAL record's seconds precision */
+			if (rec->meta_type == METATYPE_INTERVAL_WSEC) {
+				assert(rec->precision == 0);
+				rec->precision = rec->es_type->maximum_scale;
+			}
 		} else if (! dbc->no_types) {
 			/* the connection doesn't have yet the types cached (this is the
 			 * caching call) and don't have access to the data itself either,
diff --git a/test/test_conversion_sql2c_interval.cc b/test/test_conversion_sql2c_interval.cc
@@ -1260,7 +1260,7 @@ TEST_F(ConvertSQL2C_Interval, Iso86012Interval_minute_to_second)
 	ASSERT_TRUE(is.intval.day_second.fraction == 666000); // def prec: 6
 }
 
-TEST_F(ConvertSQL2C_Interval, Iso86012Interval_extra_field_22018)
+TEST_F(ConvertSQL2C_Interval, Iso86012Interval_H_in_Min2Sec)
 {
 #	undef SQL_VAL
 #	undef SQL
@@ -1284,8 +1284,12 @@ TEST_F(ConvertSQL2C_Interval, Iso86012Interval_extra_field_22018)
 	ASSERT_TRUE(SQL_SUCCEEDED(ret));
 
 	ret = SQLFetch(stmt);
-	ASSERT_FALSE(SQL_SUCCEEDED(ret));
-	assertState(L"22018");
+	ASSERT_TRUE(SQL_SUCCEEDED(ret));
+	ASSERT_TRUE(is.interval_type == SQL_IS_MINUTE_TO_SECOND);
+	ASSERT_TRUE(is.interval_sign == SQL_FALSE);
+	ASSERT_TRUE(is.intval.day_second.minute == 104);
+	ASSERT_TRUE(is.intval.day_second.second == 55);
+	ASSERT_TRUE(is.intval.day_second.fraction == 666000); // def prec: 6
 }
 
 TEST_F(ConvertSQL2C_Interval, Iso86012Interval_invalid_char_22018)
@@ -1344,6 +1348,34 @@ TEST_F(ConvertSQL2C_Interval, Iso86012Interval_repeated_valid_22018)
 	assertState(L"22018");
 }
 
+TEST_F(ConvertSQL2C_Interval, Iso86012Interval_dangling_fraction_22018)
+{
+#	undef SQL_VAL
+#	undef SQL
+#	define SQL_VAL "PT60.666S" // non-minute convertible
+#	define SQL   "SELECT " SQL_VAL
+
+	const char json_answer[] = "\
+{\
+  \"columns\": [\
+    {\"name\": \"" SQL "\", \"type\": \"INTERVAL_MINUTE\"}\
+  ],\
+  \"rows\": [\
+    [\"" SQL_VAL "\"]\
+  ]\
+}\
+";
+	prepareStatement(json_answer);
+
+	ret = SQLBindCol(stmt, /*col#*/1, SQL_C_INTERVAL_MINUTE, &is,
+			sizeof(is), &ind_len);
+	ASSERT_TRUE(SQL_SUCCEEDED(ret));
+
+	ret = SQLFetch(stmt);
+	ASSERT_FALSE(SQL_SUCCEEDED(ret));
+	assertState(L"22018");
+}
+
 TEST_F(ConvertSQL2C_Interval, Iso86012Interval_plus_minus_22018)
 {
 #	undef SQL_VAL
@@ -1540,15 +1572,6 @@ TEST_F(ConvertSQL2C_Interval, Iso8601_hour_to_second2WChar)
 			&ind_len);
 	ASSERT_TRUE(SQL_SUCCEEDED(ret));
 
-	SQLHDESC ard;
-  ret = SQLGetStmtAttr(stmt, SQL_ATTR_APP_ROW_DESC, &ard, 0, NULL);
-  ASSERT_TRUE(SQL_SUCCEEDED(ret));
-  ret = SQLSetDescField(ard, 1, SQL_DESC_PRECISION, (SQLPOINTER)3, 0);
-  ASSERT_TRUE(SQL_SUCCEEDED(ret));
-	// data ptr is reset by direct desc field setting
-  ret = SQLSetDescField(ard, 1, SQL_DESC_DATA_PTR, (SQLPOINTER)wbuff, 0);
-  ASSERT_TRUE(SQL_SUCCEEDED(ret));
-
 	ret = SQLFetch(stmt);
   ASSERT_TRUE(SQL_SUCCEEDED(ret));
   EXPECT_EQ(ind_len, sizeof(SQLWCHAR) * (sizeof("2:3:4.555") - /*\0*/1));
@@ -1578,19 +1601,11 @@ TEST_F(ConvertSQL2C_Interval, Iso8601_minute_to_second2Char)
 			&ind_len);
 	ASSERT_TRUE(SQL_SUCCEEDED(ret));
 
-	SQLHDESC ard;
-  ret = SQLGetStmtAttr(stmt, SQL_ATTR_APP_ROW_DESC, &ard, 0, NULL);
-  ASSERT_TRUE(SQL_SUCCEEDED(ret));
-  ret = SQLSetDescField(ard, 1, SQL_DESC_PRECISION, (SQLPOINTER)4, 0);
-  ASSERT_TRUE(SQL_SUCCEEDED(ret));
-	// data ptr is reset by direct desc field setting
-  ret = SQLSetDescField(ard, 1, SQL_DESC_DATA_PTR, (SQLPOINTER)buff, 0);
-  ASSERT_TRUE(SQL_SUCCEEDED(ret));
-
 	ret = SQLFetch(stmt);
-  ASSERT_TRUE(SQL_SUCCEEDED(ret));
-  EXPECT_EQ(ind_len, sizeof("3:4.5555") - /*\0*/1);
-	ASSERT_STREQ((char *)buff, "3:4.5555");
+	ASSERT_TRUE(SQL_SUCCEEDED(ret));
+	// driver truncates it to default (current) ES/SQL seconds precision
+	EXPECT_EQ(ind_len, sizeof("3:4.555") - /*\0*/1);
+	ASSERT_STREQ((char *)buff, "3:4.555");
 }
 
 TEST_F(ConvertSQL2C_Interval, Iso8601_hour_to_minute2Char)
