Add setting to turn extending numeric precision on or off #8837
Conversation
AMoo-Miki
requested review from
ananzh,
kavilla,
ashwin-pc,
joshuarrrr,
abbyhu2000,
zengyan-amazon,
zhongnansu,
manasvinibs,
ZilongX,
Flyingliuhub,
curq,
bandinib-amzn,
SuZhou-Joe,
ruanyl,
BionIT,
xinruiba,
zhyuanqi,
mengweieric,
LDrago27,
virajsanghvi,
sejli,
joshuali925 and
huyaboo
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8837 +/- ##
==========================================
+ Coverage 60.86% 60.90% +0.03%
==========================================
Files 3808 3808
Lines 91209 91212 +3
Branches 14410 14410
==========================================
+ Hits 55514 55550 +36
+ Misses 32154 32108 -46
- Partials 3541 3554 +13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
AMoo-Miki
force-pushed
the
long-numeral-switch
branch
from
79ec26e to
ce41763
Compare
| @@ -108,4 +108,5 @@ export const UI_SETTINGS = { | |||
| QUERY_DATAFRAME_HYDRATION_STRATEGY: 'query:dataframe:hydrationStrategy', | |||
| SEARCH_QUERY_LANGUAGE_BLOCKLIST: 'search:queryLanguageBlocklist', | |||
| NEW_HOME_PAGE: 'home:useNewHomePage', | |||
| DATA_WITH_LONG_NUMERALS: 'data:withLongNumerals', | |||
There was a problem hiding this comment.
nit: i think i originally did data as well. but then was convinced to use search because then we can group it with the search category. not a strong opinion on this though
There was a problem hiding this comment.
I locked in on data because this is queried by functionality not limited to search.
There was a problem hiding this comment.
sorry my pr caused a conflict on this pr so i just fixed it on github. do you mind verifying miki it's all good?
also one thing i think we should do is move this to the workspace setting or even better yet allow for request options from the query editor would make life a lot easier for developers vs an advanced setting to toggle percision or not
Thanks. Didn't see anything glaring.
Requests can choose to ignore or entertain the precision setting. I do see value for this to ALSO be in the workspace setting since not all users will have workspaces enabled. Will have to see how values are inherited from config and advanced settings to the workspace (later). Thanks for the suggestion. |
| notifications.toasts, | ||
| trackUiMetric, | ||
| history, | ||
| uiSettings, |
There was a problem hiding this comment.
uiSettings is a service, would it ever change?
There was a problem hiding this comment.
Most of these are; not sure if it is because useServicesContext can throw (which would prevent code advancing here), linting rules, or something else but most of the services fetched from the context are passed in here too. Didn't want to re-engineer this section so followed the same pattern for consistency.
| value: true, | ||
| description: i18n.translate('data.advancedSettings.data.withLongNumeralsText', { | ||
| defaultMessage: | ||
| "Turn on for precise handling of extremely large numbers. Turn off to optimize performance when high precision for large values isn't required.", |
There was a problem hiding this comment.
should this tell user what the threshold is before losing precision?
There was a problem hiding this comment.
It would be too wordy. Instead, I will have this added to the documentation website with proper explanation.
AMoo-Miki
force-pushed
the
long-numeral-switch
branch
from
649366b to
407d41e
Compare
|
@virajsanghvi thanks; just flaky test. |
* Add setting to turn extending numeric precision on or off Signed-off-by: Miki <[email protected]> * Changeset file for PR #8837 created/updated --------- Signed-off-by: Miki <[email protected]> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> (cherry picked from commit 791f5d8) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Description
Add setting to turn extending numeric precision on or off
Screenshot
Changelog
Check List
yarn test:jestyarn test:jest_integration