Skip to content

Conversation

@Hunterness
Copy link
Collaborator

@Hunterness Hunterness commented Mar 7, 2023

  • some 5.0 changes that had been missed
  • add column type

Questions for reviewers:

  • Operations: Please doublecheck I got the types right as well
  • Docs: Should the type column be between name and description instead? (I followed what we had for transaction commands so if we want to change it we should also change there)
  • Gem: Do we like the type names? Especially concerned with the LIST version as it doesn't really follow the cypher types cip (This is what we had for the show transactions command so if we want to change it I need to change there as well for consistency)

@Hunterness Hunterness added cherry-pick-this-to-5.x Cherry pick this PR changes to the 5.x branch team-cypher-operations Cypher operations should review this dev labels Mar 7, 2023
@neo-technology-commit-status-publisher
Copy link
Collaborator

Looks like you've updated the documentation!

Check out your changes at https://neo4j-docs-cypher-464.surge.sh

@gem-neo4j
Copy link
Contributor

I started writing a CIP for a typeOf function this morning, where a function will be outputting the Type as a string. In the CIP I have written my initial recommendations to match how the type system is looking (and will most likely not be final), but part of that would be someone would go through the docs and make all the types consistent with the output of this eventual function (and probably update procedure signatures in the future).

The things that stick out to me here is the decision on LISTs and MAPs, I have suggested we do LIST and RECORD to match more closely the Cypher Types CIP. You have also used LONG, which is not in the type system as such, I guess this should be INTEGER. Then DATETIME might be good as ZONED DATETIME or LOCAL DATETIME?

It would be good to get those type CIPs finalised for this 😅

@Hunterness
Copy link
Collaborator Author

LONG, LIST and MAP is already there as such for the SHOW TRANSACTION command which is why I went with it like this though I agree that it differs from the Cypher types.

I find RECORD a bit odd for MAP (I know it comes from the GQL works and I should probably add a comment on the CIP about it anyway). We also as far as I'm aware don't currently call them record but map in docs so it feels even more odd to add RECORD here (https://neo4j.com/docs/cypher-manual/current/syntax/maps/).

The datetime is new and in this case it's zoned ones, I guess I can add that but at the moment we only refer to them as DATETIME in docs in general (for example https://neo4j.com/docs/cypher-manual/current/functions/temporal/#functions-datetime) so suddenly adding ZONED also feels odd if we don't do it everywhere?

It would be good to get those type CIPs finalised for this

It sure would XD

but part of that would be someone would go through the docs and make all the types consistent with the output of this eventual function

I wonder if it is better to be consistent with what we have for now, especially since it's ongoing work, and just update it when everywhere get's updated? Cause I can see users being confused by the ZONED or RECORD and then not finding any mentions of it anywhere else in the docs 🤔

Looking further at types:

The following data types are included in the property types category: Integer, Float, String, Boolean, Point, Date, Time, LocalTime, DateTime, LocalDateTime, and Duration.
The following data types are included in the structural types category: Node, Relationship, and Path.
The following data types are included in the composite types category: List and Map.

this do argue if favor of switching LONG to INTEGER at least, but against the RECORD one. It doesn't say anything around the inner types of list so those are still up for debate I guess 🤷

@gem-neo4j
Copy link
Contributor

I agree about Map and have updated the CIPs as such :)

I think we can maybe keep as is and do one big docs update along with the implementation of the Cypher Types? Then everything is consistent at the same time

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we missed documenting options field at the time, not sure if it's a separate PR or not though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I decided it was a separate one XD

It's kinda part of a feature which feature flag is turned off and therefore undocumented. Maybe it should be documented separately excluding any mentions of the feature but it feels like a separate task from this PR

Copy link
Contributor

@ali-ince ali-ince left a comment

Choose a reason for hiding this comment

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

looks good to me.

Hunterness and others added 4 commits March 15, 2023 08:44
- error was renamed and updated to statusMessage for SHOW DATABASES
- the currentQueryIdleTime for SHOW TRANSACTIONS is a duration, not a long
Co-authored-by: Ali Ince <ali-ince@users.noreply.github.com>
@Hunterness Hunterness force-pushed the dev-show-column-corrections branch from a109b7e to 1f76042 Compare March 15, 2023 07:44
Copy link
Collaborator

@JPryce-Aklundh JPryce-Aklundh left a comment

Choose a reason for hiding this comment

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

Looks great @Hunterness!
I just have a few suggestions.

As for your question, I don't think moving Type between Column and Description is necessary in this case. As far as I can see, the improvement would only be very marginal (e.g. where the value is BOOLEAN, it would maybe make sense for readers to know that before a description that says "if true" in front of it, but it is still coherent and comprehensible as it stands).

@Hunterness
Copy link
Collaborator Author

As for your question, I don't think moving Type between Column and Description is necessary in this case. As far as I can see, the improvement would only be very marginal

@JPryce-Aklundh The thing that triggered the question for me was the default labels in the description which is in the middle of the table, where previously they had been at the end. But I'm fine leaving it as is as well.

Hunterness and others added 2 commits March 15, 2023 17:09
Co-authored-by: Jens Pryce-Åklundh <112686610+JPryce-Aklundh@users.noreply.github.com>
@JPryce-Aklundh JPryce-Aklundh merged commit f65dab0 into neo4j:dev Mar 17, 2023
@JPryce-Aklundh JPryce-Aklundh deleted the dev-show-column-corrections branch March 17, 2023 09:22
JPryce-Aklundh added a commit that referenced this pull request Mar 17, 2023
Original PR: #464

Cherry-pick excludes 5.6 specific content in original PR

Co-authored-by: Therese Magnusson <therese.magnusson@neotechnology.com>
lidiazuin pushed a commit to lidiazuin/docs-cypher that referenced this pull request May 21, 2025
Original PR: neo4j#464

Cherry-pick excludes 5.6 specific content in original PR

Co-authored-by: Therese Magnusson <therese.magnusson@neotechnology.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cherry-pick-this-to-5.x Cherry pick this PR changes to the 5.x branch dev team-cypher-operations Cypher operations should review this

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants