Skip to content

Add quotes around printed domain values#16208

Merged
mbasmanova merged 1 commit intoprestodb:masterfrom
lightseba:master
Jun 9, 2021
Merged

Add quotes around printed domain values#16208
mbasmanova merged 1 commit intoprestodb:masterfrom
lightseba:master

Conversation

@lightseba
Copy link
Contributor

@lightseba lightseba commented Jun 4, 2021

The domain intervals can be ambiguous in cases like
SELECT * FROM table WHERE col >= 'a' AND col <= 'b'
vs.
SELECT * FROM table WHERE col = 'a, b'
which both have the domain interval [a, b].
This is fixed by adding quotes to the output (["a", "b"] vs. ["a, b"])
and escaping quotes which may appear in the string itself.
Also made PlanPrinter.formatDomain() use the formatting code in
Range.toString(), which is exactly the same.

Test plan
In addition to the unit tests added, I ran

CREATE TABLE test_orders WITH (partitioned_by = ARRAY['orderkey', 'processing']) AS 
SELECT custkey, orderkey, orderstatus = 'P' processing FROM orders WHERE orderkey < 3
EXPLAIN SELECT * FROM test_orders WHERE orderkey < 2 AND orderkey > 20

This query should show quotes around the domain intervals.

== RELEASE NOTES ==

General Changes
* Add double quotes around the column domain values in the text query plan. 
  Literal double quotes in values will be escaped with a backslash (ab"c -> ab\"c).

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 4, 2021

CLA Signed

The committers are authorized under a signed CLA.

  • ✅ lightseba (e8bfe573f0feaa0743362098b3abe5d2500732f0)

@lightseba lightseba requested a review from ahmadghazal64 June 4, 2021 16:18
Copy link
Contributor

@kaikalur kaikalur Jun 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about if if have field value as '<min>' or '<max>'? Please add a test for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor

@ahmadghazal64 ahmadghazal64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we talked, please add tests for lower level API range and higher level PlanPrinter. For each, try both cases of x in ('a','b') and x in ('a,b')

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 7, 2021

CLA Signed

The committers are authorized under a signed CLA.

  • ✅ lightseba (e1af5e7a292708cf4f03d912dd5a6f13f55d774c)

@kaikalur kaikalur requested a review from mbasmanova June 7, 2021 21:59
@mbasmanova
Copy link
Contributor

@lightseba This change has a user-visible impact. Please, add release notes to the PR description.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the logic of adding quotes is repeated multiple times; consider adding a helper method that takes a buffer and a value to add and adds the value with quotes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move this variable to top of the class; consider adding line breaks for readability, e.g. before .setTimeZoneKey, .setLegacyTimestamp, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@lightseba lightseba requested a review from mbasmanova June 7, 2021 22:37
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

val -> marker

The domain intervals can be ambiguous in cases like
SELECT * FROM table WHERE col >= 'a' AND col <= 'b'
vs.
SELECT * FROM table WHERE col = 'a, b'
which both have the domain interval [a, b].
This is fixed by adding quotes to the output (["a", "b"] vs. ["a, b"])
and escaping quotes which may appear in the string itself.
Also made PlanPrinter.formatDomain() use the formatting code in
Range.toString(), which is exactly the same.
@mbasmanova mbasmanova merged commit a2efda5 into prestodb:master Jun 9, 2021
@mayankgarg1990 mayankgarg1990 mentioned this pull request Jun 18, 2021
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants