Repository navigation
feat: Visualize complex Hive column types v2 - #1091
Conversation
|
I've extended the length of In the event that a column type is still longer than the new limit, everything degrades gracefully and the partial type string will be short as before. |
jczhong84
left a comment
jczhong84
left a comment
There was a problem hiding this comment.
Looks good to me overall, thanks for the update!
Also could you update the version in package.json?
| if (!matches || matches.length < 3) { | ||
| return { key, type }; | ||
| } |
There was a problem hiding this comment.
is this to handle the truncated case?
- could you add a test case for it?
- what if the truncated type also matches the regex? like
struct<date:struct<hour:int>
There was a problem hiding this comment.
Added a case for non-matching truncated, and the 2nd scenario where the truncated type matches the regex accidentally. I've addressed this by checking the depth field to ensure we've closed all <> pairs
| } | ||
| */ | ||
| export function parseType(key: string, type: string): ComplexType { | ||
| const regex = /(struct|array|map|uniontype)<(.*)>/i; |
There was a problem hiding this comment.
does it need to handle spaces? like struct <...>, struct<year : int, day : int>?
also should it match the start and end /^ ... $/?
There was a problem hiding this comment.
I left spaces out intentionally as I haven't seen that in the wild. If needed it probably wouldn't be too hard, but didn't want to add unnecessary complexity.
There was a problem hiding this comment.
Added start/end to the regex.
| if (char === '<') { | ||
| prettyString += '<\n'; | ||
| depth += 1; | ||
| prettyString += ' '.repeat(depth); |
There was a problem hiding this comment.
could you use a variable for the 2 spaces ' '?
| {complexType.type} | ||
| </StyledText> | ||
| <AccentText weight="extra">{complexType.key}</AccentText> |
There was a problem hiding this comment.
instead of displaying it as type key, I'd prefer to have it same as the ColumnRow, which is key type, which I think is more reasonable.
645b79f to
a030692
Compare
| updateDataColumnDescription, | ||
| }) => { | ||
| const [expanded, setExpanded] = React.useState(false); | ||
| const parsedType = parseType('', column.type); |
| ); | ||
| }; | ||
|
|
||
| interface IDataTableColumnCardNestedTypeProps { |
There was a problem hiding this comment.
can you separate this into another file?
| IDataTableColumnCardNestedTypeProps | ||
| > = ({ complexType }) => { | ||
| const hasChildren = complexType.children?.length > 0; | ||
| const [expanded, setExpanded] = React.useState(false); |
There was a problem hiding this comment.
you can use useToggleState then you don't need () => setExpanded(!expanded)
| const hasChildren = complexType.children?.length > 0; | ||
| const [expanded, setExpanded] = React.useState(false); | ||
|
|
||
| const rowProps = { |
There was a problem hiding this comment.
const rowProps: React.HTMLAttributes = {}
| </div> | ||
| {hasChildren && | ||
| expanded && | ||
| complexType.children?.map((child) => ( |
| 'struct<ids:array<string>,data:uniontype<int,float,string>>' | ||
| ) | ||
| ).toEqual(`struct< | ||
| ids:array< |
There was a problem hiding this comment.
should we add a space after : to make it easier to read?
| onClick={onColumnRowClick.bind(null, col.id)} | ||
| name={col.name} | ||
| type={col.type} | ||
| typeChildren={parseType('', col.type).children} |
There was a problem hiding this comment.
memo this?
actually, the parsing can be down in ColumnRow since col.type is passed as a prop
There was a problem hiding this comment.
ColumnRow takes typeChildren because it only parses once, then recursively output nested rows. I've moved the parseType call down and inside useMemo, where it returns either the passed typeChildren or calculates it.
e5e26b5 to
963482e
Compare
|
Rebased to resolve conflicts / squashed the commits. |
3 participants
This is an alternate version of #1072, replacing the JSON viewer with a more natural-looking nested type drilldown. I've also addressed some issues with the original version.
Compared to #1072, the parser is more consistent and does a better job of handling certain types than before. To support the nesting, it always creates nodes with optional children. Now
array<>anduniontype<>types will create placeholder<element>nodes underneath them, andmap<>types create placeholder<key>and<value>nodes.Complex type parsing is case-insensitive (e.g. struct vs STRUCT) but otherwise case is preserved.
There are three main features: