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

Remove string encoding enum #183

Closed
wants to merge 2 commits into from

Conversation

kgpai
Copy link
Contributor

@kgpai kgpai commented Sep 9, 2021

With the recent ascii compute changes (See: #32), the StringEncoding enum is redundant and no longer required.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 9, 2021
@kgpai kgpai force-pushed the remove_ascii_encoding branch from 5c0dca1 to 03f9324 Compare September 9, 2021 18:38
Copy link
Contributor

@mbasmanova mbasmanova left a comment

Choose a reason for hiding this comment

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

Thanks.

case StringEncodingMode::MOSTLY_ASCII:
Func<StringEncodingMode::MOSTLY_ASCII>::apply(args...);
return;
static void apply(const bool mode, Params... args) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: mode -> isAscii for clarity

@kgpai kgpai force-pushed the remove_ascii_encoding branch from 03f9324 to 5fc2f06 Compare September 9, 2021 18:59
@facebook-github-bot
Copy link
Contributor

@kgpai has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@kgpai kgpai force-pushed the remove_ascii_encoding branch from 36992b1 to f02ad11 Compare September 10, 2021 15:56
@facebook-github-bot
Copy link
Contributor

@kgpai has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@kgpai merged this pull request in 0243740.

simuthus-fb added a commit to simuthus-fb/velox that referenced this pull request Jan 24, 2023
Summary:
X-link: facebookincubator/katran#183

X-link: facebook/openr#143

X-link: facebook/fbthrift#536

X-link: facebook/fb303#33

X-link: facebook/folly#1924

FBOSS OSS on-diff job is failing due to random test failures.

Disabling the tests for now to avoid build breakages.

Reviewed By: shri-khare

Differential Revision: D42550176

fbshipit-source-id: fb887cd092adae0a57f7a377fa7e1941c46c5afd
PHILO-HE pushed a commit to PHILO-HE/velox that referenced this pull request Apr 10, 2023
* fix bloom filter validation

Encounter this error when we run q93, we increase settings "spark.sql.optimizer.runtime.bloomFilter.maxNumItems" for our tests. Constants check happens before min comparsion, so when next grouping comes, it failed with not constant value error.
---------

Co-authored-by: zhli1142015 <zhli@pczhlich.fareast.corp.microsoft.com>
rui-mo pushed a commit to rui-mo/velox that referenced this pull request Apr 23, 2023
* fix bloom filter validation

Encounter this error when we run q93, we increase settings "spark.sql.optimizer.runtime.bloomFilter.maxNumItems" for our tests. Constants check happens before min comparsion, so when next grouping comes, it failed with not constant value error.
---------

Co-authored-by: zhli1142015 <zhli@pczhlich.fareast.corp.microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants