Skip to content

Commit

Permalink
Fix chunk skipping range check bug
Browse files Browse the repository at this point in the history 
Chunk skipping based on column stats had a convoluted way
of detecting if query clauses were hitting a chunk in certain
cases. This change makes things more straight forward and
adds missing test cases for range checking.
  • Loading branch information
@antekresic
antekresic committed Oct 7, 2024
1 parent ae030ef commit f9973a9
Show file tree
Hide file tree
Showing 8 changed files with 893 additions and 207 deletions.
1 change: 1 addition & 0 deletions .unreleased/pr_7318
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fixes: #7318: Fix chunk skipping range filtering
83 changes: 4 additions & 79 deletions src/hypertable_restrict_info.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -175,81 +175,6 @@ dimension_restrict_info_open_add(DimensionRestrictInfoOpen *dri, StrategyNumber
return restriction_added;
}

/*
* for DIMENSION_TYPE_STATS entries, we need to use regular bounded strategies.
* We can have multiple entries satisfying the inputs. It basically becomes a
* problem of:
*
* "given an input value, find all ranges which encompass that value"
*
* For "column >= constant" (e.g id >= 9), the check has to be:
*
* start_range >= 9 OR end_range >= 9 (second check covers +INF)
*
* For "column <= constant" (e.g id <= 90), the check has to be:
*
* start_range <= 90 OR end_range <= 90 (first check covers -INF)
*
* For "column == constant" (e.g id = 9)
*
* start_range <= 9 OR end_range >= 9 (covers +INF to -INF)
*/
static bool
dimension_restrict_info_range_add(DimensionRestrictInfoOpen *dri, StrategyNumber strategy,
Oid collation, DimensionValues *dimvalues)
{
ListCell *item;
bool restriction_added = false;

/* can't handle IN/ANY with multiple values */
if (dimvalues->use_or && list_length(dimvalues->values) > 1)
return false;

foreach (item, dimvalues->values)
{
Oid restype;
Datum datum = ts_dimension_transform_value(dri->base.dimension,
collation,
PointerGetDatum(lfirst(item)),
dimvalues->type,
&restype);
int64 value = ts_time_value_to_internal_or_infinite(datum, restype);

switch (strategy)
{
case BTLessEqualStrategyNumber:
case BTLessStrategyNumber: /* e.g: id <= 90 */
if (dri->upper_strategy == InvalidStrategy || value < dri->upper_bound)
{
dri->upper_strategy = strategy;
dri->upper_bound = value;
restriction_added = true;
}
break;
case BTGreaterEqualStrategyNumber:
case BTGreaterStrategyNumber: /* e.g: id >= 9 */
if (dri->lower_strategy == InvalidStrategy || value > dri->lower_bound)
{
dri->lower_strategy = strategy;
dri->lower_bound = value;
restriction_added = true;
}
break;
case BTEqualStrategyNumber: /* e.g: id = 9 */
dri->lower_bound = value;
dri->upper_bound = value;
dri->lower_strategy = BTEqualStrategyNumber;
dri->upper_strategy = BTEqualStrategyNumber;
restriction_added = true;
break;
default:
/* unsupported strategy */
break;
}
}
return restriction_added;
}

static List *
dimension_restrict_info_get_partitions(DimensionRestrictInfoClosed *dri, Oid collation,
List *values)
Expand Down Expand Up @@ -330,10 +255,10 @@ dimension_restrict_info_add(DimensionRestrictInfo *dri, int strategy, Oid collat
values);
case DIMENSION_TYPE_STATS:
/* we reuse the DimensionRestrictInfoOpen structure for these */
return dimension_restrict_info_range_add((DimensionRestrictInfoOpen *) dri,
strategy,
collation,
values);
return dimension_restrict_info_open_add((DimensionRestrictInfoOpen *) dri,
strategy,
collation,
values);
case DIMENSION_TYPE_CLOSED:
return dimension_restrict_info_closed_add((DimensionRestrictInfoClosed *) dri,
strategy,
Expand Down
89 changes: 8 additions & 81 deletions src/ts_catalog/chunk_column_stats.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1071,7 +1071,6 @@ ts_chunk_column_stats_get_chunk_ids_by_scan(DimensionRestrictInfo *dri)
ScanIterator it;
List *chunkids = NIL;
DimensionRestrictInfoOpen *open;
bool check_both = false;

Assert(dri && dri->dimension->type == DIMENSION_TYPE_STATS);

Expand All @@ -1082,25 +1081,6 @@ ts_chunk_column_stats_get_chunk_ids_by_scan(DimensionRestrictInfo *dri)

open = (DimensionRestrictInfoOpen *) dri;

/*
* Since we need to find chunks with overlapping ranges. We use checks on both the
* lower_bound and the upper_bound.
*/
if (open->upper_strategy != InvalidStrategy && open->lower_strategy != InvalidStrategy)
check_both = true;

if (open->upper_strategy == InvalidStrategy)
{
open->upper_strategy = open->lower_strategy;
open->upper_bound = open->lower_bound;
}

if (open->lower_strategy == InvalidStrategy)
{
open->lower_strategy = open->upper_strategy;
open->lower_bound = open->upper_bound;
}

/*
* We need to get all chunks matching the hypertable ID and the column name.
*/
Expand Down Expand Up @@ -1143,18 +1123,6 @@ ts_chunk_column_stats_get_chunk_ids_by_scan(DimensionRestrictInfo *dri)
goto done;
}

/*
* If both upper and lower bounds have been specified then we need to check
* if that range overlaps (subset, superset, intersect) with the current fd entry
* values
*/
if (check_both)
{
/* range is before or after our fd range_start/range_end values */
if (open->upper_bound < fd->range_start || open->lower_bound > fd->range_end)
goto done;
}

/*
* All data is in int8 format so we do regular comparisons. Also, it's an OR
* check so prepare to short circuit if one evaluates to true.
Expand All @@ -1166,78 +1134,37 @@ ts_chunk_column_stats_get_chunk_ids_by_scan(DimensionRestrictInfo *dri)
{
case BTLessEqualStrategyNumber: /* e.g: id <= 90 */
{
/* range_end is exclusive, so check accordingly */
if ((fd->range_end - 1) <= open->upper_bound)
matched = check_both ? false : true;
matched = fd->range_start <= open->upper_bound;
}
break;
case BTLessStrategyNumber: /* e.g: id < 90 */
{
/* range_end is exclusive, so check accordingly */
if ((fd->range_end - 1) < open->upper_bound)
matched = check_both ? false : true;
}
break;
case BTGreaterEqualStrategyNumber: /* e.g: id >= 90 */
{
/* range_end is exclusive, so check accordingly */
if ((fd->range_end - 1) >= open->upper_bound)
matched = check_both ? false : true;
}
break;
case BTGreaterStrategyNumber: /* e.g: id > 9 */
{
/* range_end is exclusive, so check accordingly */
if ((fd->range_end - 1) > open->upper_bound)
matched = check_both ? false : true;
}
break;
case BTEqualStrategyNumber: /* e.g: id == 9 */
{
/* need to check for both range_start and range_end */
if (fd->range_start <= open->lower_bound &&
(fd->range_end - 1) >= open->upper_bound)
matched = true;
matched = fd->range_start < open->upper_bound;
}
break;
default:
/* unsupported strategy */
open->upper_strategy = InvalidStrategy;
break;
}

if (matched)
if (open->upper_strategy != InvalidStrategy && !matched)
goto done;

/* range_end checks didn't match, check for range_start now */
switch (open->lower_strategy)
{
case BTLessEqualStrategyNumber:
{
if (fd->range_start <= open->lower_bound)
matched = true;
}
break;
case BTLessStrategyNumber:
{
if (fd->range_start < open->lower_bound)
matched = true;
}
break;
case BTGreaterEqualStrategyNumber:
{
if (fd->range_start >= open->lower_bound)
matched = true;
/* range_end is exclusive */
matched = (fd->range_end - 1) >= open->lower_bound;
}
break;
case BTGreaterStrategyNumber:
{
/* range_start is inclusive */
if (fd->range_start >= open->lower_bound)
matched = true;
/* range_end is exclusive */
matched = (fd->range_end - 1) > open->lower_bound;
}
break;
case BTEqualStrategyNumber:
/* already handled above with upper_strategy */
default:
/* unsupported strategy */
break;
Expand Down
Loading

0 comments on commit f9973a9

Please sign in to comment.