Skip to content

Conversation

@huboneo
Copy link
Contributor

@huboneo huboneo commented Dec 2, 2019

This PR introduces a new configuration option in browser for truncating max field length before they are passed to rendering layers. The reason for this is that certain queries can produce a single result with a lot of values in a given field, which we had yet to account for.

Screenshot

Screenshot 2019-12-12 at 13 01 29

Screenshot 2020-01-07 at 12 30 25

Copy link
Member

@oskarhane oskarhane left a comment

Choose a reason for hiding this comment

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

Some really good stuff in here.

Some comments:

  • I get somewhat inconsistent results. If you have the movies database and set the "Max field items" to 1 and execute MATCH p=()-[r:ACTED_IN]->() RETURN p LIMIT 25 I see two nodes in the viz.
    But switching to table view I see 25 node objects.

  • The limitations we already have with "Max Rows" in table view + "Initial node display" in viz, maybe those should be removed and replaced by this one (since this operates at a lower level)?

  • I don't see a truncation message in the viz's statusbar.

  • Maybe not part of this PR, but just to point it out, property size is also a performance killer (try returning a node with a property of 1MB).

},
{
maxFieldItems: {
displayName: 'Max field items',
Copy link
Member

Choose a reason for hiding this comment

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

"field items" is not very clear what it means. Maybe "Item Limit" or or something to make it more clear?
"Field" isn't a word we regularly use except deep in the driver response.

Comment on lines +77 to +81
const resultJson = JSON.stringify(
request.result.records,
fieldLimiterFactory(maxFieldItems),
2
)
Copy link
Member

Choose a reason for hiding this comment

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

Since this only does JSON.stringify and only executes when the user switches to this tab, can we skip the truncation here?
The purpose of the code tab/view is to give the developer a window into what's being returned from the driver without any formatting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately not, even JSON.stringify() gets overwhelmed with large result fields

expect(out.nodes.length).toEqual(4)
})
test('should find items in paths with segments', () => {
test('should find items in paths with segments, and only return unique items', () => {
Copy link
Member

Choose a reason for hiding this comment

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

👍

@huboneo
Copy link
Contributor Author

huboneo commented Jan 7, 2020

@oskarhane thank you for your feedback.

  • Indeed there was a bug truncation.
  • Added a message to Vis for truncated messages
  • Updated setting display name
  • Unsure about removing max row truncation as that is on a different axis from fields. Lets leave it as is for now?

@huboneo huboneo requested a review from oskarhane January 7, 2020 11:36
Copy link
Member

@oskarhane oskarhane left a comment

Choose a reason for hiding this comment

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

Thanks for the update on this.

Result bug

I still get mismatches between the different view though.
To reproduce:

  1. Load the common movie dataset
  2. Set the limit to 1
  3. Run MATCH p=(n:Movie)-[]-(:Person) RETURN p LIMIT 25 and compare the views. The table view only sees one item per row while viz and text show the whole path.
    See:

Screenshot 2020-01-13 09 27 13

Phrasing

I've re-tought the phrasing as well and I think it's ok to use the word "fields" as this is the first time we describe the records width to users. So sorry about the back-and-forth on that.
I've suggest what wording to use via comments.

<Ellipsis>
{this.state.hasTruncatedFields && (
<StyledTruncatedMessage>
<Icon name='warning sign' /> Result fields have been
Copy link
Member

Choose a reason for hiding this comment

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

"Result fields" => "Records fields"

},
{
maxFieldItems: {
displayName: 'Property item limit',
Copy link
Member

Choose a reason for hiding this comment

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

"Record field limit" describes this better imo. wdyt?

maxFieldItems: {
displayName: 'Property item limit',
tooltip:
'Max amount of items shown for list properties. When reached, lists get truncated.'
Copy link
Member

@oskarhane oskarhane Jan 13, 2020

Choose a reason for hiding this comment

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

"Max number of fields per record being handled" is more clear imo. wdyt?

<StyledInspectorFooterRowListValue className="value">
{this.props.hasTruncatedFields && (
<StyledTruncatedMessage>
<Icon name="warning sign" /> Result fields have been
Copy link
Member

Choose a reason for hiding this comment

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

"Result fields" => "Records fields"

@huboneo
Copy link
Contributor Author

huboneo commented Jan 13, 2020

@oskarhane thank you again for your feedback. I have updated the PR. Tests are still failing I believe

@huboneo
Copy link
Contributor Author

huboneo commented Jan 13, 2020

Screenshot 2020-01-13 at 10 51 47

Copy link
Member

@oskarhane oskarhane left a comment

Choose a reason for hiding this comment

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

Thanks for the update!

I still see different results in table vs text view. They should be the same. The viz does some magic with internal relationships, but not the different table views.

  1. Load movie dataset
  2. Set limit to 1
  3. Run MATCH p=()-[r:FOLLOWS]->() RETURN p LIMIT 25 and compare table and text views.

Screenshot 2020-01-13 12 23 11

👍 on the wording changes.

@oskarhane
Copy link
Member

Stable version of react-table is now out @huboneo. When yo have time, could you update this upstream and update this PR?

huboneo added 7 commits April 24, 2020 12:33
- Introduced new setting "maxFieldItems"
- Now truncating data passed to CypherFrames whenever a field that is too large is detected
- Refactored boltMapping code to increase performance
- Side effect is that we now dedupe nodes, not sure if OK
@huboneo
Copy link
Contributor Author

huboneo commented Apr 29, 2020

closing in favour of #1090

@huboneo huboneo closed this Apr 29, 2020
@huboneo huboneo deleted the perf-issue-wip branch April 29, 2020 09:54
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.

3 participants