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

Fix values of products.min_price field in Kibana sample ecommerce data set #90428

Merged
merged 15 commits into from
Feb 15, 2021

Conversation

przemekwitek
Copy link
Contributor

@przemekwitek przemekwitek commented Feb 5, 2021

This PR adds .01 suffix to the products.min_price integer values so that all the products.min_price values are floating point numbers.
This change prevents dynamic mappings to kick in when reindexing or transforming this index.

cloning.ts and editing.ts functional tests are not re-enabled as part of this PR. This will be handled separately.

Relates elastic/elasticsearch#67148

@przemekwitek przemekwitek added WIP Work in progress release_note:enhancement v7.12.0 v8.0.0 Feature:Home Kibana home application and removed WIP Work in progress labels Feb 5, 2021
@przemekwitek przemekwitek marked this pull request as ready for review February 5, 2021 13:55
@przemekwitek przemekwitek requested a review from a team as a code owner February 5, 2021 13:55
@darnautov
Copy link
Contributor

hey @przemekwitek, thanks for creating a PR!
We also need to fix the ecommerce dataset in ML tests, you can find it here.
I wrote functional tests for the latest transform, they are disabled at the moment because of this issue. You can enable them here and here and they should pass.
cc @pheyos

@darnautov darnautov requested a review from pheyos February 8, 2021 08:21
@pheyos
Copy link
Member

pheyos commented Feb 8, 2021

@przemekwitek @darnautov We should update both of our ecommerce based test datasets (ecommerce and module_sample_ecommerce). Could be done as part of this PR or, if we want to keep the scope small, I can do it in a follow-up PR.

@przemekwitek przemekwitek requested a review from a team as a code owner February 8, 2021 10:31
@przemekwitek
Copy link
Contributor Author

We also need to fix the ecommerce dataset in ML tests, you can find it here.

Done.

I wrote functional tests for the latest transform, they are disabled at the moment because of this issue. You can enable them here and here and they should pass.

Done.

We should update both of our ecommerce based test datasets (ecommerce and module_sample_ecommerce). Could be done as part of this PR or, if we want to keep the scope small, I can do it in a follow-up PR.

Done as part of this PR.

@darnautov
Copy link
Contributor

darnautov commented Feb 8, 2021

@przemekwitek might be you need to adjust mappings here

Edit: it's half_float already 🤔

@pheyos
Copy link
Member

pheyos commented Feb 9, 2021

The test archive data changes are looking good, but we still get the following error in the ES log when the latest transform is run:

org.elasticsearch.xpack.transform.transforms.BulkIndexingException: Bulk index experienced [1] failures and at least 1 irrecoverable [mapper [products.min_price] cannot be changed from type [long] to [float]]

I couldn't find the root cause of this yet.

@przemekwitek
Copy link
Contributor Author

The test archive data changes are looking good, but we still get the following error in the ES log when the latest transform is run:

org.elasticsearch.xpack.transform.transforms.BulkIndexingException: Bulk index experienced [1] failures and at least 1 irrecoverable [mapper [products.min_price] cannot be changed from type [long] to [float]]

I couldn't find the root cause of this yet.

I couldn't find the root cause either.
Mappings of the index look ok. What is not ok is that sometimes this field in source has integer value, e.g.: 46 . I changed all integers to floats (46.0) but somehow this change is not reflected in kibana_sample_data_ecommerce index. This index still sometimes has integer values.
On the other hand, when I upload my updated dataset to Kibana as JSON, it's working fine.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link
Member

@pheyos pheyos left a comment

Choose a reason for hiding this comment

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

Test data change LGTM

Copy link
Contributor

@darnautov darnautov left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for fixing the data sets!

@przemekwitek przemekwitek merged commit 9ba9128 into elastic:master Feb 15, 2021
jloleysens added a commit to jloleysens/kibana that referenced this pull request Feb 15, 2021
…ndition-for-hiding-recommded-allocation

* 'master' of github.com:elastic/kibana:
  [Discover] Fix toggling multi fields from doc view table (elastic#91121)
  [ML] Data Frame Analytics: ROC Curve Chart (elastic#89991)
  skip flaky suite (elastic#86948)
  skip flaky suite (elastic#91191)
  Fix date histogram time zone for rollup index (elastic#90632)
  [Search Source] Fix retrieval of unmapped fields; Add field filters (elastic#89837)
  [Logs UI] Use useMlHref hook for ML links (elastic#90935)
  Fix values of `products.min_price` field in Kibana sample ecommerce data set (elastic#90428)
  [APM] Darker shade for Error group details labels (elastic#91349)
  [Lens] Adjust new copy for 7.12 (elastic#90413)
  [ML] Unskip test. Fix modelMemoryLimit value. (elastic#91280)
  [Lens] Fix empty display name issue in XY chart (elastic#91132)
  [Lens] Improves error messages when in Dashboard (elastic#90668)
  [Lens] Keyboard-selected items follow user traversal of drop zones (elastic#90546)
  [Lens] Improves ranking feature in Top values (elastic#90749)
  [ILM] Rollover min age tooltip and copy fixes (elastic#91110)

# Conflicts:
#	x-pack/plugins/index_lifecycle_management/__jest__/client_integration/edit_policy/edit_policy.test.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants