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

Add explore operator and table format (TUI) #3113

Open
wants to merge 84 commits into
base: main
Choose a base branch
from
Open

Conversation

mavam
Copy link
Member

@mavam mavam commented May 1, 2023

This PR adds a tui operator that renders pipeline results in a terminal user interface (TUI).

The PR is continuation of #2838, leveraging the FTXUI library.

See the docs page for details.

Screenshot:

image (2)

Review instructions:

  • Review the commit that adds record_type::depth() separately.
  • Go through the rest file-by-file, with the operator plugin as entry point.
  • If you need help on understanding FTXUI, I'm happy to pair on it.
  • Make sure the CMake looks right.

@mavam mavam force-pushed the topic/tui-take-two branch 2 times, most recently from 71c1f31 to 61d8134 Compare May 3, 2023 20:04
@mavam mavam marked this pull request as ready for review May 3, 2023 20:39
@mavam mavam requested a review from dominiklohmann May 3, 2023 20:39
@mavam
Copy link
Member Author

mavam commented May 19, 2023

@tobim FTXUI just released v4.1, which works with this PR. IIRC this is easier to bundle with Nix than a specific SHA. Can you help me with getting that going?

@mavam
Copy link
Member Author

mavam commented May 22, 2023

Nix now has FTXUI 4.1 per this PR: NixOS/nixpkgs#232288

Now we need to wire it here.

@tobim
Copy link
Member

tobim commented May 22, 2023

I would prefer to do the nixpkgs bump in a separate PR. Can I push a fix to this branch to decouple the dependency?

@mavam
Copy link
Member Author

mavam commented May 22, 2023

@otbim I will just rebase this branch once your other branch is merged. If you could push the Nix changes on this branch afterwards, that would be helpful.

@mavam mavam force-pushed the topic/tui-take-two branch 4 times, most recently from 71ac1ab to 4ee45fc Compare May 25, 2023 15:13
@tobim
Copy link
Member

tobim commented May 30, 2023

The Docker build fails with:

2023-05-30T12:47:27.3302465Z #23 1331.2 [505/746] Building CXX object plugins/tui/aux/ftxui/CMakeFiles/dom.dir/src/ftxui/dom/canvas.cpp.o
2023-05-30T12:47:27.4812936Z #23 1331.2 FAILED: plugins/tui/aux/ftxui/CMakeFiles/dom.dir/src/ftxui/dom/canvas.cpp.o 
2023-05-30T12:47:27.4814701Z #23 1331.2 /usr/bin/g++-12 -Ddom_EXPORTS -I/tmp/vast/plugins/tui/aux/ftxui/src -isystem /tmp/vast/plugins/tui/aux/ftxui/include -O3 -DNDEBUG -fPIC -Wall -Wextra -pedantic -Werror -Wmissing-declarations -Wdeprecated -Wshadow -std=c++20 -MD -MT plugins/tui/aux/ftxui/CMakeFiles/dom.dir/src/ftxui/dom/canvas.cpp.o -MF plugins/tui/aux/ftxui/CMakeFiles/dom.dir/src/ftxui/dom/canvas.cpp.o.d -o plugins/tui/aux/ftxui/CMakeFiles/dom.dir/src/ftxui/dom/canvas.cpp.o -c /tmp/vast/plugins/tui/aux/ftxui/src/ftxui/dom/canvas.cpp
2023-05-30T12:47:27.4815762Z #23 1331.2 In file included from /usr/include/c++/12/string:40,
2023-05-30T12:47:27.4816205Z #23 1331.2                  from /tmp/vast/plugins/tui/aux/ftxui/include/ftxui/dom/canvas.hpp:6,
2023-05-30T12:47:27.4816672Z #23 1331.2                  from /tmp/vast/plugins/tui/aux/ftxui/src/ftxui/dom/canvas.cpp:1:
2023-05-30T12:47:27.4817493Z #23 1331.2 In static member function 'static constexpr std::char_traits<char>::char_type* std::char_traits<char>::copy(char_type*, const char_type*, std::size_t)',
2023-05-30T12:47:27.4818544Z #23 1331.2     inlined from 'static constexpr void std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::_S_copy(_CharT*, const _CharT*, size_type) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]' at /usr/include/c++/12/bits/basic_string.h:423:21,
2023-05-30T12:47:27.4820121Z #23 1331.2     inlined from 'constexpr std::__cxx11::basic_string<_CharT, _Traits, _Allocator>& std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::_M_replace(size_type, size_type, const _CharT*, size_type) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]' at /usr/include/c++/12/bits/basic_string.tcc:532:22,
2023-05-30T12:47:27.4821397Z #23 1331.2     inlined from 'constexpr std::__cxx11::basic_string<_CharT, _Traits, _Alloc>& std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::assign(const _CharT*) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]' at /usr/include/c++/12/bits/basic_string.h:1647:19,
2023-05-30T12:47:27.4822585Z #23 1331.2     inlined from 'constexpr std::__cxx11::basic_string<_CharT, _Traits, _Alloc>& std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::operator=(const _CharT*) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]' at /usr/include/c++/12/bits/basic_string.h:815:28,
2023-05-30T12:47:27.4823641Z #23 1331.2     inlined from 'void ftxui::Canvas::DrawBlockOn(int, int)' at /tmp/vast/plugins/tui/aux/ftxui/src/ftxui/dom/canvas.cpp:469:30,
2023-05-30T12:47:27.4824370Z #23 1331.2     inlined from 'void ftxui::Canvas::DrawBlockOn(int, int)' at /tmp/vast/plugins/tui/aux/ftxui/src/ftxui/dom/canvas.cpp:462:6:
2023-05-30T12:47:27.4825375Z #23 1331.2 /usr/include/c++/12/bits/char_traits.h:431:56: error: 'void* __builtin_memcpy(void*, const void*, long unsigned int)' accessing 9223372036854775810 or more bytes at offsets -4611686018427387902 and [-4611686018427387903, 4611686018427387904] may overlap up to 9223372036854775813 bytes at offset -3 [-Werror=restrict]
2023-05-30T12:47:27.4826042Z #23 1331.2   431 |         return static_cast<char_type*>(__builtin_memcpy(__s1, __s2, __n));
2023-05-30T12:47:27.4826446Z #23 1331.2       |                                        ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~
2023-05-30T12:47:27.4827145Z #23 1331.2 In static member function 'static constexpr std::char_traits<char>::char_type* std::char_traits<char>::copy(char_type*, const char_type*, std::size_t)',
2023-05-30T12:47:27.4828180Z #23 1331.2     inlined from 'static constexpr void std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::_S_copy(_CharT*, const _CharT*, size_type) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]' at /usr/include/c++/12/bits/basic_string.h:423:21,
2023-05-30T12:47:27.4829433Z #23 1331.2     inlined from 'constexpr std::__cxx11::basic_string<_CharT, _Traits, _Allocator>& std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::_M_replace(size_type, size_type, const _CharT*, size_type) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]' at /usr/include/c++/12/bits/basic_string.tcc:532:22,
2023-05-30T12:47:27.4830674Z #23 1331.2     inlined from 'constexpr std::__cxx11::basic_string<_CharT, _Traits, _Alloc>& std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::assign(const _CharT*) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]' at /usr/include/c++/12/bits/basic_string.h:1647:19,
2023-05-30T12:47:27.4831844Z #23 1331.2     inlined from 'constexpr std::__cxx11::basic_string<_CharT, _Traits, _Alloc>& std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::operator=(const _CharT*) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]' at /usr/include/c++/12/bits/basic_string.h:815:28,
2023-05-30T12:47:27.4832755Z #23 1331.2     inlined from 'void ftxui::Canvas::DrawBlockOff(int, int)' at /tmp/vast/plugins/tui/aux/ftxui/src/ftxui/dom/canvas.cpp:488:30,
2023-05-30T12:47:27.4833494Z #23 1331.2     inlined from 'void ftxui::Canvas::DrawBlockOff(int, int)' at /tmp/vast/plugins/tui/aux/ftxui/src/ftxui/dom/canvas.cpp:482:6:
2023-05-30T12:47:27.4834595Z #23 1331.2 /usr/include/c++/12/bits/char_traits.h:431:56: error: 'void* __builtin_memcpy(void*, const void*, long unsigned int)' accessing 9223372036854775810 or more bytes at offsets -4611686018427387902 and [-4611686018427387903, 4611686018427387904] may overlap up to 9223372036854775813 bytes at offset -3 [-Werror=restrict]
2023-05-30T12:47:27.4835257Z #23 1331.2   431 |         return static_cast<char_type*>(__builtin_memcpy(__s1, __s2, __n));
2023-05-30T12:47:27.4835658Z #23 1331.2       |                                        ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~
2023-05-30T12:47:27.4836353Z #23 1331.2 In static member function 'static constexpr std::char_traits<char>::char_type* std::char_traits<char>::copy(char_type*, const char_type*, std::size_t)',
2023-05-30T12:47:27.4837389Z #23 1331.2     inlined from 'static constexpr void std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::_S_copy(_CharT*, const _CharT*, size_type) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]' at /usr/include/c++/12/bits/basic_string.h:423:21,
2023-05-30T12:47:27.4838727Z #23 1331.2     inlined from 'constexpr std::__cxx11::basic_string<_CharT, _Traits, _Allocator>& std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::_M_replace(size_type, size_type, const _CharT*, size_type) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]' at /usr/include/c++/12/bits/basic_string.tcc:532:22,
2023-05-30T12:47:27.4839965Z #23 1331.2     inlined from 'constexpr std::__cxx11::basic_string<_CharT, _Traits, _Alloc>& std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::assign(const _CharT*) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]' at /usr/include/c++/12/bits/basic_string.h:1647:19,
2023-05-30T12:47:27.4841118Z #23 1331.2     inlined from 'constexpr std::__cxx11::basic_string<_CharT, _Traits, _Alloc>& std::__cxx11::basic_string<_CharT, _Traits, _Alloc>::operator=(const _CharT*) [with _CharT = char; _Traits = std::char_traits<char>; _Alloc = std::allocator<char>]' at /usr/include/c++/12/bits/basic_string.h:815:28,
2023-05-30T12:47:27.4842029Z #23 1331.2     inlined from 'void ftxui::Canvas::DrawBlockToggle(int, int)' at /tmp/vast/plugins/tui/aux/ftxui/src/ftxui/dom/canvas.cpp:509:30:
2023-05-30T12:47:27.4843017Z #23 1331.2 /usr/include/c++/12/bits/char_traits.h:431:56: error: 'void* __builtin_memcpy(void*, const void*, long unsigned int)' accessing 9223372036854775810 or more bytes at offsets -4611686018427387902 and [-4611686018427387903, 4611686018427387904] may overlap up to 9223372036854775813 bytes at offset -3 [-Werror=restrict]
2023-05-30T12:47:27.4843667Z #23 1331.2   431 |         return static_cast<char_type*>(__builtin_memcpy(__s1, __s2, __n));
2023-05-30T12:47:27.4844075Z #23 1331.2       |                                        ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~

Curiously, the regular Debian build worked.

@mavam
Copy link
Member Author

mavam commented May 31, 2023

I also have no trouble on macOS. Weird. Could this be a GCC bug?

@mavam mavam force-pushed the topic/tui-take-two branch 3 times, most recently from 476f4d8 to e619f2c Compare June 14, 2023 11:30
@mavam mavam added operator Source, transformation, and sink feature New functionality and removed enhancement labels Jul 2, 2023
Copy link
Member

@dominiklohmann dominiklohmann left a comment

Choose a reason for hiding this comment

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

This is a tremendous feat. I really like it. I mostly noted down things relating to the user experience for now, and did not yet look much at the code.

Copy link
Member

Choose a reason for hiding this comment

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

(Unrelated to this document, just wanted to have a comment that can be resolved.)

Take a look at this screenshot:

image

The border color of the header is off. It sometimes matches the field name color, and sometimes the type name color.

Copy link
Member Author

Choose a reason for hiding this comment

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

This happens because the border colors slightly change based on the focused pane, to support guiding the user where they are. I might remove this detail unless I get the inner borders to update as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will be addressed in a future PR.

Copy link
Member

Choose a reason for hiding this comment

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

Where do we track this?

web/docs/operators/sinks/explore.md Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

(Unrelated to this document, just wanted to have a comment that can be resolved.)

Having many output schemas does not render well:

image

Copy link
Member Author

Choose a reason for hiding this comment

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

Not something I'll be able to address in this PR, but it'll go on a list of follow-up improvements.

Copy link
Member

Choose a reason for hiding this comment

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

Same question as above: Where do we track this? Maybe add a list of known issues to the code itself?

web/docs/operators/sinks/explore.md Show resolved Hide resolved
web/docs/operators/sinks/explore.md Show resolved Hide resolved
plugins/explore/src/components.cpp Outdated Show resolved Hide resolved
plugins/explore/src/components.cpp Outdated Show resolved Hide resolved
plugins/explore/src/plugin.cpp Outdated Show resolved Hide resolved
plugins/explore/src/plugin.cpp Outdated Show resolved Hide resolved
web/docs/operators/sinks/explore.md Show resolved Hide resolved
@mavam mavam linked an issue Jul 23, 2023 that may be closed by this pull request
@mavam mavam changed the title Add explore operator (TUI) Add explore operator and table format (TUI) Jul 23, 2023
This commit introduces a new shared library with components used by the
`explore` and `table` plugins. This new library contains all shared
logic, e.g., UI components and DOM elements.
@mavam
Copy link
Member Author

mavam commented Aug 6, 2023

The provided functionality of this PR is at a point where we should merge it. The remaining task is getting the packaging in place. It's unclear that we'll get a release, and release cycles generally are not short.

@tobim I'd say we figure out how to a custom version that points to a specific commit, just like in our submodule. What does this entail?

EDIT: looks like we're going to get a new FTXUI v5.0 release in the next few weeks, so let's keep sitting on our fingers. 🚀

@mavam
Copy link
Member Author

mavam commented Aug 17, 2023

Here's a solution for redirecting stdin, which is probably something we want at some point as it's the natural source for data: ArthurSonzogni/FTXUI#723 (comment)

@mavam
Copy link
Member Author

mavam commented Aug 23, 2023

@tobim I've bumped FTXUI to v5.0.0. This was the last piece needed to get this PR in shape functionally. I'm just struggling again with the build setup. Any help to get things compile in CI would be terrific. 🙏

@dominiklohmann
Copy link
Member

This needs to be rebased; with FTXUI 5.0 being out, do we still need the submodule at all for this?

@mavam
Copy link
Member Author

mavam commented Sep 11, 2023

do we still need the submodule at all for this?

I think so. It's not packaged on Homebrew and neither Debian.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New functionality operator Source, transformation, and sink
Projects
None yet
4 participants