[#18360] YSQL: Rework yb_hash_code comparision with an out of range c…

…onstants

Summary:
Rework solution from https://phorge.dev.yugabyte.com/D36446 to substitute undesired
narrow_cast and some implicit cast with propper functions signature
Jira: DB-7345

Test Plan: Jenkins

Reviewers: jason, amartsinchyk

Reviewed By: jason, amartsinchyk

Subscribers: yql

Tags: #jenkins-ready

Differential Revision: https://phorge.dev.yugabyte.com/D36858

java/yb-pgsql/src/test/java/org/yb/pgsql/TestPgYbHashCodeScanProjection.java

@@ -160,21 +160,24 @@ private static void verifyPartialScanIterBytesRead(
   // @param selectList The SELECT-list items used in the queries
   // @param expectedNumColumns Expected number of items in the column_refs DocDB request field
   // @param maxHashCode The yb_hash_code predicate upper bound value to use in the query.
+  // @param expectedMaxHashCodeInRequest The upper bound for hash code in request sent to t-server
   private void testOneCase(
-      Statement stmt, String selectList, int expectedNumColumns, int maxHashCode)
+      Statement stmt, String selectList, int expectedNumColumns,
+      int maxHashCode, int expectedMaxHashCodeInRequest)
       throws Exception {
     final ScanInfo seqScan = collectSeqScanInfo(stmt, selectList);
     final ScanInfo indexScan = collectFullIndexScanInfo(stmt, selectList);
     final ScanInfo hashScan = collectHashCodeScanInfo(stmt, selectList, maxHashCode);
 
-    final double scanFraction = (double)(maxHashCode + 1) / (kYbHashCodeMax + 1);
+    final double scanFraction =
+        (double)(Math.min(maxHashCode, kYbHashCodeMax) + 1) / (kYbHashCodeMax + 1);
     final double errorTolerance = (maxHashCode >= kYbHashCodeMax? 0.0: kErrorTolerance);
 
     final String message = String.format(
         "selectList=[%s] maxHashCode=%d seqScan: %s indexScan: %s hashScan: %s",
         selectList, maxHashCode, seqScan, indexScan, hashScan);
 
-    assertEquals(message, maxHashCode, hashScan.maxHashCode);
+    assertEquals(message, expectedMaxHashCodeInRequest, hashScan.maxHashCode);
     assertEquals(message, expectedNumColumns, hashScan.projection.size());
 
     verifyPartialScanIterBytesRead(
@@ -184,19 +187,32 @@ private void testOneCase(
       message, indexScan.iterBytesRead, scanFraction, hashScan.iterBytesRead, errorTolerance);
   }
 
+  private void testOneCase(
+      Statement stmt, String selectList, int expectedNumColumns, int maxHashCode)
+      throws Exception {
+    testOneCase(stmt, selectList, expectedNumColumns, maxHashCode, maxHashCode);
+  }
+
   @Test
   public void testScans() throws Exception {
     try (Statement stmt = connection.createStatement()) {
       // Note: In case of using yb_hash_code function all its argument columns are fetched.
       //       They are required for row recheck.
 
       // full range
-      testOneCase(stmt, "z", 2, kYbHashCodeMax);
-      testOneCase(stmt, "z, d", 3, kYbHashCodeMax);
-      testOneCase(stmt, "*", kNumTableColumns, kYbHashCodeMax);
-      testOneCase(stmt, "k", 1, kYbHashCodeMax);  // the hash key
-      testOneCase(stmt, "0", 1, kYbHashCodeMax);  // no table column
-      testOneCase(stmt, "yb_hash_code(k)", 1, kYbHashCodeMax);
+      testOneCase(stmt, "z", 2, kYbHashCodeMax, kNoHashCode);
+
+      // more than full range
+      testOneCase(stmt, "z", 2, kYbHashCodeMax + 1, kNoHashCode);
+
+      // almost full range
+      final int maxHashCode = kYbHashCodeMax - 1;
+      testOneCase(stmt, "z", 2, maxHashCode);
+      testOneCase(stmt, "z, d", 3, maxHashCode);
+      testOneCase(stmt, "*", kNumTableColumns, maxHashCode);
+      testOneCase(stmt, "k", 1, maxHashCode);  // the hash key
+      testOneCase(stmt, "0", 1, maxHashCode);  // no table column
+      testOneCase(stmt, "yb_hash_code(k)", 1, maxHashCode);
 
       // partial range
       testOneCase(stmt, "z", 2, 32767); // 1/2

src/postgres/src/backend/access/yb_access/yb_scan.c

@@ -23,6 +23,7 @@
 
 #include <limits.h>
 #include <math.h>
+#include <stdint.h>
 #include <string.h>
 #include "postgres.h"
 
@@ -1217,9 +1218,9 @@ static int64 YbDatumGetInt64(Datum value, Oid value_typid)
 	switch (value_typid)
 	{
 		case INT2OID:
-			return (int64) DatumGetInt16(value);
+			return DatumGetInt16(value);
 		case INT4OID:
-			return (int64) DatumGetInt32(value);
+			return DatumGetInt32(value);
 		case INT8OID:
 			return DatumGetInt64(value);
 		default:
@@ -2246,22 +2247,28 @@ YbPredetermineNeedsRecheck(Relation relation,
 
 typedef struct {
 	YBCPgBoundType type;
-	int64 value;
+	int64_t value;
 } YbBound;
 
+static inline bool
+YbBoundEqual(const YbBound *lhs, const YbBound *rhs)
+{
+	return lhs->type == rhs->type && lhs->value == rhs->value;
+}
+
 typedef struct {
 	YbBound start;
 	YbBound end;
 } YbRange;
 
 static inline bool
-YbBoundValid(const YbBound* bound)
+YbBoundValid(const YbBound *bound)
 {
 	return bound->type != YB_YQL_BOUND_INVALID;
 }
 
 static inline bool
-YbBoundInclusive(const YbBound* bound)
+YbBoundInclusive(const YbBound *bound)
 {
 	return bound->type == YB_YQL_BOUND_VALID_INCLUSIVE;
 }
@@ -2276,23 +2283,16 @@ YbIsValidRange(const YbBound *start, const YbBound *end)
 	        YbBoundInclusive(end));
 }
 
-#define YB_HASH_CODE_MAX 65535
-
 static bool
 YbApplyStartBound(YbRange *range, const YbBound *start)
 {
+	Assert(YbIsValidRange(&range->start, &range->end));
 	Assert(YbBoundValid(start));
 
-	if (start->value < 0)
-		return true;
-	if (start->value > YB_HASH_CODE_MAX)
-		return false;
-
-	if (YbBoundValid(&range->end) && !YbIsValidRange(start, &range->end))
+	if (!YbIsValidRange(start, &range->end))
 		return false;
 
-	if (!YbBoundValid(&range->start) ||
-	    (range->start.value < start->value) ||
+	if ((range->start.value < start->value) ||
 	    (range->start.value == start->value && !YbBoundInclusive(start)))
 	{
 		range->start = *start;
@@ -2303,30 +2303,37 @@ YbApplyStartBound(YbRange *range, const YbBound *start)
 static bool
 YbApplyEndBound(YbRange *range, const YbBound *end)
 {
+	Assert(YbIsValidRange(&range->start, &range->end));
 	Assert(YbBoundValid(end));
 
-	if (end->value < 0)
-		return false;
-	if (end->value > YB_HASH_CODE_MAX)
-		return true;
-
-	if (YbBoundValid(&range->start) && !YbIsValidRange(&range->start, end))
+	if (!YbIsValidRange(&range->start, end))
 		return false;
 
-	if (!YbBoundValid(&range->end) ||
-	    (range->end.value > end->value) ||
+	if ((range->end.value > end->value) ||
 	    (range->end.value == end->value && !YbBoundInclusive(end)))
 	{
 		range->end = *end;
 	}
 	return true;
 }
 
+static inline uint16_t
+YbBoundUint16Value(const YbBound *bound)
+{
+	Assert(bound->type == YB_YQL_BOUND_INVALID ||
+		   (bound->value >= 0 && bound->value <= UINT16_MAX));
+	return bound->value;
+}
+
 static bool
 YbBindHashKeys(YbScanDesc ybScan)
 {
-	ListCell   *lc;
-	YbRange		range = {0};
+	static const YbBound YB_MIN_HASH_BOUND =
+		{.type = YB_YQL_BOUND_VALID_INCLUSIVE, .value = 0};
+	static const YbBound YB_MAX_HASH_BOUND =
+		{.type = YB_YQL_BOUND_VALID_INCLUSIVE, .value = UINT16_MAX};
+	YbRange range = {.start = YB_MIN_HASH_BOUND, .end = YB_MAX_HASH_BOUND};
+	ListCell *lc;
 
 	foreach(lc, ybScan->hash_code_keys)
 	{
@@ -2339,9 +2346,9 @@ YbBindHashKeys(YbScanDesc ybScan)
 		switch (key->sk_strategy)
 		{
 			case BTEqualStrategyNumber:
-					bound.type = YB_YQL_BOUND_VALID_INCLUSIVE;
-					if (!YbApplyStartBound(&range, &bound) ||
-						!YbApplyEndBound(&range, &bound))
+				bound.type = YB_YQL_BOUND_VALID_INCLUSIVE;
+				if (!YbApplyStartBound(&range, &bound) ||
+					!YbApplyEndBound(&range, &bound))
 						return false;
 				break;
 
@@ -2366,11 +2373,15 @@ YbBindHashKeys(YbScanDesc ybScan)
 		}
 	}
 
+	if (YbBoundEqual(&range.start, &YB_MIN_HASH_BOUND))
+		range.start = (YbBound){};
+	if (YbBoundEqual(&range.end, &YB_MAX_HASH_BOUND))
+		range.end = (YbBound){};
 	if (YbBoundValid(&range.start) || YbBoundValid(&range.end))
-		HandleYBStatus(YBCPgDmlBindHashCodes(
+		YBCPgDmlBindHashCodes(
 			ybScan->handle,
-			range.start.type, range.start.value,
-			range.end.type, range.end.value));
+			range.start.type, YbBoundUint16Value(&range.start),
+			range.end.type, YbBoundUint16Value(&range.end));
 
 	return true;
 }

src/yb/yql/pggate/pg_dml_read.cc

@@ -849,13 +849,13 @@ bool PgDmlRead::IsAllPrimaryKeysBound() const {
   return true;
 }
 
-Status PgDmlRead::BindHashCode(const std::optional<Bound>& start, const std::optional<Bound>& end) {
+void PgDmlRead::BindHashCode(const std::optional<Bound>& start, const std::optional<Bound>& end) {
   if (secondary_index_query_) {
-    return secondary_index_query_->BindHashCode(start, end);
+    secondary_index_query_->BindHashCode(start, end);
+    return;
   }
   ApplyBound(read_req_.get(), start, true /* is_lower */);
   ApplyBound(read_req_.get(), end, false /* is_lower */);
-  return Status::OK();
 }
 
 Status PgDmlRead::BindRange(

src/yb/yql/pggate/pg_dml_read.h

@@ -91,7 +91,7 @@ class PgDmlRead : public PgDml {
   // Bind a column with an IS NOT NULL condition.
   Status BindColumnCondIsNotNull(int attr_num);
 
-  Status BindHashCode(const std::optional<Bound>& start, const std::optional<Bound>& end);
+  void BindHashCode(const std::optional<Bound>& start, const std::optional<Bound>& end);
 
   // Limit scan to specific ybctid range for parallel scan.
   // Sets underlying request's bounds to specified values, also resets any psql operations

src/yb/yql/pggate/pggate.cc

@@ -1409,9 +1409,9 @@ Status PgApiImpl::DmlAddRowLowerBound(YBCPgStatement handle,
                                                         is_inclusive);
 }
 
-Status PgApiImpl::DmlBindHashCode(
+void PgApiImpl::DmlBindHashCode(
   PgStatement* handle, const std::optional<Bound>& start, const std::optional<Bound>& end) {
-  return down_cast<PgDmlRead*>(handle)->BindHashCode(start, end);
+  down_cast<PgDmlRead*>(handle)->BindHashCode(start, end);
 }
 
 Status PgApiImpl::DmlBindRange(YBCPgStatement handle,

src/yb/yql/pggate/pggate.h

@@ -472,7 +472,7 @@ class PgApiImpl {
   Status DmlBindColumnCondIsNotNull(PgStatement *handle, int attr_num);
   Status DmlBindRow(YBCPgStatement handle, uint64_t ybctid, YBCBindColumn* columns, int count);
 
-  Status DmlBindHashCode(
+  void DmlBindHashCode(
       PgStatement* handle, const std::optional<Bound>& start, const std::optional<Bound>& end);
 
   Status DmlBindRange(YBCPgStatement handle,

src/yb/yql/pggate/ybc_pggate.cc

@@ -196,12 +196,11 @@ Slice YbctidAsSlice(uint64_t ybctid) {
   return pgapi->GetYbctidAsSlice(ybctid);
 }
 
-inline std::optional<Bound> MakeBound(YBCPgBoundType type, uint64_t value) {
+inline std::optional<Bound> MakeBound(YBCPgBoundType type, uint16_t value) {
   if (type == YB_YQL_BOUND_INVALID) {
     return std::nullopt;
   }
-  return Bound{.value = narrow_cast<uint16_t>(value),
-               .is_inclusive = (type == YB_YQL_BOUND_VALID_INCLUSIVE)};
+  return Bound{.value = value, .is_inclusive = (type == YB_YQL_BOUND_VALID_INCLUSIVE)};
 }
 
 Status InitPgGateImpl(const YBCPgTypeEntity* data_type_table,
@@ -1332,10 +1331,13 @@ YBCStatus YBCPgDmlBindColumnCondIn(YBCPgStatement handle, YBCPgExpr lhs, int n_a
 
 YBCStatus YBCPgDmlBindHashCodes(
   YBCPgStatement handle,
-  YBCPgBoundType start_type, int start_value,
-  YBCPgBoundType end_type, int end_value) {
-  return ToYBCStatus(pgapi->DmlBindHashCode(
-      handle, MakeBound(start_type, start_value), MakeBound(end_type, end_value)));
+  YBCPgBoundType start_type, uint16_t start_value,
+  YBCPgBoundType end_type, uint16_t end_value) {
+  const auto start = MakeBound(start_type, start_value);
+  const auto end = MakeBound(end_type, end_value);
+  DCHECK(start || end);
+  pgapi->DmlBindHashCode(handle, start, end);
+  return YBCStatusOK();
 }
 
 YBCStatus YBCPgDmlBindRange(YBCPgStatement handle,

src/yb/yql/pggate/ybc_pggate.h

@@ -505,9 +505,10 @@ YBCStatus YBCPgDmlBindRow(
 
 YBCStatus YBCPgDmlGetColumnInfo(YBCPgStatement handle, int attr_num, YBCPgColumnInfo* info);
 
-YBCStatus YBCPgDmlBindHashCodes(YBCPgStatement handle,
-                                YBCPgBoundType start_type, int start_value,
-                                YBCPgBoundType end_type, int end_value);
+YBCStatus YBCPgDmlBindHashCodes(
+    YBCPgStatement handle,
+    YBCPgBoundType start_type, uint16_t start_value,
+    YBCPgBoundType end_type, uint16_t end_value);
 
 // For parallel scan only, limit fetch to specified range of ybctids
 YBCStatus YBCPgDmlBindRange(YBCPgStatement handle,