Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[YQL-17103] Support StartsWith with pg types in extract_predicate library #854

Merged
merged 4 commits into from
Jan 9, 2024

Conversation

nepal
Copy link
Collaborator

@nepal nepal commented Jan 5, 2024

  • support StartsWith predicates for pg types

@nepal nepal requested a review from vitstn January 5, 2024 16:56
Copy link

github-actions bot commented Jan 5, 2024

Note

This is an automated comment that will be appended during run.

🔴 linux-x86_64-relwithdebinfo: some tests FAILED for commit eaba788.

Test history

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
59867 50645 0 12 9205 5

🔴 linux-x86_64-release-asan: some tests FAILED for commit eaba788.

Test history

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
15859 15719 0 23 102 15

@nepal nepal force-pushed the yql-17103-support-pg-startswith branch from 085a2e5 to 0e213c4 Compare January 5, 2024 20:23
@@ -4665,6 +4665,16 @@ void RegisterCoSimpleCallables1(TCallableOptimizerMap& map) {
map["IsDistinctFrom"] = std::bind(&OptimizeDistinctFrom<false>, _1, _2);

map["StartsWith"] = map["EndsWith"] = map["StringContains"] = [](const TExprNode::TPtr& node, TExprContext& ctx, TOptimizeContext& /*optCtx*/) {
if (node->Head().GetTypeAnn()->GetKind() == ETypeAnnotationKind::Pg || node->Tail().GetTypeAnn()->GetKind() == ETypeAnnotationKind::Pg) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why this overload is needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This overload helps to simplify logic of predicate analysis. This way we make StartsWith with pg types to look exactly the same as with plain YQL types (no intermediate FromPg is needed), so the code which looks for StartsWith over Member() call will work without pg-specific tweaks.

Also, accepting PG types here looks like a nice feature to me.

Copy link
Collaborator

@vitstn vitstn left a comment

Choose a reason for hiding this comment

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

need to discuss

@nepal nepal force-pushed the yql-17103-support-pg-startswith branch from 0e213c4 to da312f3 Compare January 6, 2024 19:53
Copy link

github-actions bot commented Jan 6, 2024

❌ Documentation build

Revision build failed

Build logs

Errors (8)

❌ Error occurred in /en/postgresql/functions.md

❌ Error occurred in /en/reference/ydb-cli/workload-kv.md

❌ Error occurred in /ru/postgresql/functions.md

❌ Error occurred in /ru/reference/ydb-cli/workload-kv.md

❌ No such file or has no access to /en/postgresql/functions.md

❌ No such file or has no access to /en/reference/ydb-cli/workload-kv.md

❌ No such file or has no access to /ru/postgresql/functions.md

❌ No such file or has no access to /ru/reference/ydb-cli/workload-kv.md

@nepal
Copy link
Collaborator Author

nepal commented Jan 6, 2024

need to discuss

Discussed and decided

  1. add test for StartsWith/EndsWith/StringContains with pg types
  2. document new overload in docs

```

Обязательные аргументы:

* Исходная строка;
* Искомая подстрока.

Аргументы могут быть типов `String` или `Utf8` и могут быть опциональными.
Аргументы должны иметь тип `String`/`Utf8` (или опциональный String`/`Utf8`) либо строковый PostgreSQL тип (`PgText`/`PgBinary`/`PgVarchar`).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Pgbytea

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

@nepal nepal force-pushed the yql-17103-support-pg-startswith branch from da312f3 to eaba788 Compare January 9, 2024 13:36
Copy link

github-actions bot commented Jan 9, 2024

✅ Documentation build

Revision builded successful
Revision preview link

@nepal nepal requested a review from vitstn January 9, 2024 14:16
@nepal nepal enabled auto-merge (squash) January 9, 2024 19:28
@nepal nepal merged commit 2706eed into ydb-platform:main Jan 9, 2024
3 of 5 checks passed
@nepal nepal deleted the yql-17103-support-pg-startswith branch January 9, 2024 19:58
@niksaveliev niksaveliev mentioned this pull request Jan 11, 2024
This was referenced Jan 11, 2024
@pavelvelikhov pavelvelikhov mentioned this pull request Feb 3, 2024
@niksaveliev niksaveliev mentioned this pull request Feb 5, 2024
@vitstn vitstn mentioned this pull request Feb 16, 2024
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.

2 participants