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

CSV upload indexes FALSE values are truthy #7977

Closed
Bargs opened this issue Aug 10, 2016 · 6 comments
Closed

CSV upload indexes FALSE values are truthy #7977

Bargs opened this issue Aug 10, 2016 · 6 comments
Labels
bug Fixes for quality problems that affect the customer experience Feature:Add Data Add Data and sample data feature on Home

Comments

@Bargs
Copy link
Contributor

Bargs commented Aug 10, 2016

If a CSV contains boolean values in all caps (TRUE/FALSE), Elasticsearch will index "FALSE" as a truthy value. This is because the sever side CSV parser does not recognize FALSE as a boolean value, and sends it as a JSON string. To make matters worse, the client side parser DOES detect it as a boolean value, so the user will think everything is working fine until they realize every FALSE value has been indexed as true.

I can think of a couple ways to fix this:

  • Turn off auto type detection on the server side and use the client side selections. We'd just have to normalize things like geo_point to their JSON values. This is probably the correct solution.
  • Somehow normalize capitalization of boolean values.
@Bargs Bargs added bug Fixes for quality problems that affect the customer experience P2 Feature:Add Data Add Data and sample data feature on Home labels Aug 10, 2016
@epixa
Copy link
Contributor

epixa commented Aug 10, 2016

What's the value of the server-side transformation over simply doing it on the client?

@Bargs
Copy link
Contributor Author

Bargs commented Aug 10, 2016

Right now we rely on the browser to upload the raw file contents and parse it on the server in a streaming manner. It allows us to completely avoid loading the file contents into memory on the client side, except for the first few lines to show the preview. If we do the transformation to JSON on the client we'll need to stream those parse results to the backend to avoid breaking the browser on large files. I'm not sure how to do that without sending many small http requests (I tried this but it made the browser really laggy and increased ingest time) or websockets (complicated).

After giving this some thought though, I'm not sure either of my previous suggestions are the right solution. I'm not so sure we want to modify the user's data at all in the CSV upload wizard. If we detect TRUE and FALSE strings as boolean, but elasticsearch does not, that might cause even more confusion. Strictness would make things more predictable. But the downside is that Excel exports bools as TRUE/FALSE, so this is going to be a common issue.

@ck-lee
Copy link
Contributor

ck-lee commented Aug 17, 2016

What about showing a warning message to the user on preview, similar to the issue with white space on header fields? That way, users can decide what they want to do with it.

@Bargs
Copy link
Contributor Author

Bargs commented Aug 17, 2016

I think the downside to creating a warning like that is, where do you stop? This is one weird case, but there are lots of others too. For instance, is null falsey? Some users might expect it to be, but elasticsearch says no. Whereas the string values "no" and "off" are considered falsey. Providing warnings will require Kibana to have an intimate knowledge of what values are considered true/false by ES.

Some day I'd like to enhance the pattern configuration step to index the test data and show the user how it's actually going to be seen by ES. But that's a longer term solution, I'm not sure what a good short term solution is.

@ck-lee
Copy link
Contributor

ck-lee commented Aug 18, 2016

I see what you mean. An actual ES import preview is a good idea! And I think it is worth the effort here. I personally find that CSV import feature has great potential for Kibana. Good luck with it. I am happy to help out too.

@epixa
Copy link
Contributor

epixa commented Dec 26, 2016

I'm going to close this since CSV upload was pulled. We can always refer back to this issue if/when we revisit the feature.

@epixa epixa closed this as completed Dec 26, 2016
Ikuni17 pushed a commit that referenced this issue Aug 31, 2024
`v95.7.0` ⏩ `v95.9.0`

> [!CAUTION]
> This PR contains the final set of Emotion conversions for all EuiForm
components.
> If your plugin contains any custom CSS/styling to **EuiFormRow,
EuiFormControlLayout, EuiCheckbox, EuiRadio, or EuiSwitch**, ⚠️ make
sure to QA your UI to ensure no visual regressions have occurred! ⚠️

---

## [`v95.9.0`](https://github.com/elastic/eui/releases/v95.9.0)

- Updated `EuiSearchBar`'s optional `box.schema` prop with a new
`recognizedFields` configuration. This allows specifying the phrases
that will be parsed as field clauses
([#7960](elastic/eui#7960))
- Updated `EuiIcon` with a new `tokenSemanticText` glyph
([#7971](elastic/eui#7971))
- Added support for TypeScript 5
([#7980](elastic/eui#7980))

**Bug fixes**

- Fixed `EuiSelectableTemplateSitewide` styles when used within a
dark-themed `EuiHeader`
([#7977](elastic/eui#7977))

## [`v95.8.0`](https://github.com/elastic/eui/releases/v95.8.0)

- Updated `EuiHeaderLinks`'s mobile menu to set a slight popover padding
by default ([#7961](elastic/eui#7961))
- This can be overridden via `popoverProps.panelPaddingSize` if needed.
- Updated `EuiHeaderLink` to default to a size of `s` (down from `m`)
([#7961](elastic/eui#7961))

**Accessibility**

- Updated the `aria-label` attribute for the `EuiFieldSearch` clear
button ([#7970](elastic/eui#7970))

**Bug fixes**

- Fixed a visual bug with `<EuiDualRange showInput="inputWithPopover"
/>` form controls ([#7957](elastic/eui#7957))

**Deprecations**

- Deprecated `EuiFormRow`'s `columnCompressedSwitch` display prop. Use
`columnCompressed` instead, which will automatically account for child
`EuiSwitch`es ([#7968](elastic/eui#7968))
- Deprecated `EuiFormRow`'s `rowCompressed` display prop. Use `row`
instead for vertical forms, or `centerCompressed` for inline forms
([#7968](elastic/eui#7968))
- (Styling) Updated `EuiFormRow`'s `hasEmptySpaceLabel` prop to no
longer attempt to automatically align its content to a vertical center.
Use the `display="center"` prop for that instead
([#7968](elastic/eui#7968))

**CSS-in-JS conversions**

- Converted `EuiFormControlLayout` to Emotion
([#7954](elastic/eui#7954))
- Removed `.euiFormControlLayout--*icons` classNames and
`--eui-form-control-layout-icons-padding` CSS var. Use
`--euiFormControlRightIconsCount` or `--euiFormControlLeftIconsCount`
instead
- Converted `EuiFormLayoutDelimited` to Emotion
([#7957](elastic/eui#7957))
- Fixed `cloneElementWithCss` throwing an error when used multiple times
without a `key` prop ([#7957](elastic/eui#7957))
- Updated `cloneElementWithCss` utility to support a third argument that
allows prepending vs. appending the cloned Emotion css className
([#7957](elastic/eui#7957))
- Removed `@euiFormControlLayoutClearIcon` Sass mixin
([#7959](elastic/eui#7959))
- Converted `EuiDescribedFormGroup` to Emotion
([#7964](elastic/eui#7964))
- Converted `EuiForm`, `EuiFormHelpText`, and `EuiFormErrorText` to
Emotion ([#7966](elastic/eui#7966))
- Converted `EuiFormLabel` and `EuiFormLegend` to Emotion; Removed
`@euiFormLabel` mixin
([#7967](elastic/eui#7967))
- Converted `EuiFormRow` to Emotion
([#7968](elastic/eui#7968))
- Converted `EuiCheckbox` to Emotion
([#7969](elastic/eui#7969))
- Converted `EuiRadio` to Emotion
([#7969](elastic/eui#7969))
- Converted `EuiSwitch` to Emotion
([#7969](elastic/eui#7969))
- Removed the following Sass variables:
([#7969](elastic/eui#7969))
  - `$euiFormCustomControlDisabledIconColor`
  - `$euiFormCustomControlBorderColor`
  - `$euiRadioSize`
  - `$euiCheckBoxSize`
  - `$euiCheckboxBorderRadius`
  - `$euiSwitchHeight` (and compressed/mini variants)
  - `$euiSwitchWidth` (and compressed/mini variants)
  - `$euiSwitchThumbSize` (and compressed/mini variants)
  - `$euiSwitchIconHeight`
  - `$euiSwitchOffColor`
- Removed the following Sass mixins:
([#7969](elastic/eui#7969))
  - `euiIconBackground`
  - `euiCustomControl`
  - `euiCustomControlSelected`
  - `euiCustomControlDisabled`
  - `euiCustomControlFocused`

---------

Co-authored-by: Marta Bondyra <4283304+mbondyra@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:Add Data Add Data and sample data feature on Home
Projects
None yet
Development

No branches or pull requests

3 participants