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

review for existing error messages #1713

Merged
merged 9 commits into from
Nov 14, 2024
Merged

Conversation

amorton
Copy link
Contributor

@amorton amorton commented Nov 13, 2024

Which issue(s) this PR fixes:
Fixes #1662

Checklist

  • Changes manually tested
  • Automated Tests added/updated
  • Documentation added/updated
  • CLA Signed: DataStax CLA

@amorton amorton requested a review from a team as a code owner November 13, 2024 23:59
CANNOT_SORT_TABLE_UPDATE_COMMAND,
CANNOT_SORT_TOO_MUCH_DATA,
CANNOT_SORT_VECTOR_AND_NON_VECTOR_COLUMNS,
UNSUPPORTED_SORT_FOR_TABLE_DELETE_COMMAND,
Copy link
Contributor

Choose a reason for hiding this comment

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

Not in alphabetic order?

Copy link
Contributor

@tatu-at-datastax tatu-at-datastax left a comment

Choose a reason for hiding this comment

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

Makes sense: added suggestions, many/most which could be done as follow-up.

@@ -157,7 +157,7 @@ request-errors:
# NOTE: UNKNOWN_TABLE_COLUMNS is also in the FILTER scope
- scope: DOCUMENT
code: UNKNOWN_TABLE_COLUMNS
title: Unknown table columns in document
title: Columns in documents are not defined in the table schema
Copy link
Contributor

Choose a reason for hiding this comment

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

should it be "Columns in document" (not plural since exceptions are per-document/row).
Also: do we talk about "documents" with tables as well? Could just leave out document here altogether ("Columns are not defined in the table schema").

@@ -168,7 +168,7 @@ request-errors:

- scope: DOCUMENT
code: UNSUPPORTED_COLUMN_TYPES
title: Unsupported table column data types in document
title: Column types in documents are not supported
Copy link
Contributor

Choose a reason for hiding this comment

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

same here (singular document; document vs row; maybe just leave "document(s)" out altogether)

code: FILTER_REQUIRED_FOR_UPDATE_DELETE
title: A filter is required for Update and DeleteOne commands
code: MISSING_FILTER_FOR_UPDATE_DELETE
title: Update and delete commands require a filter
body: |-
An Update or DeleteOne command was issued without a filter.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it be "Update or Delete command"? (or "UpdateOne" or "DeleteOne")

code: INVALID_IN_FILTER_FOR_UPDATE_ONE_DELETE_ONE
title: In filter is not allowed for UpdateOne and DeleteOne commands
code: UNSUPPORTED_IN_FILTER_FOR_UPDATE_ONE_DELETE_ONE
title: Update one and delete one commands do not support `$in` or `$nin` filter operation
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove "one"s, just like above (-> "Update and delete commands...")

code: ZERO_UPDATE_OPERATIONS_FOR_TABLE
title: Cannot update with zero operations
code: MISSING_UPDATE_OPERATIONS
title: Update operation requires at least one operation
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it be "Update command requires..." (otherwise "operation requires operation")

@@ -590,7 +593,7 @@ request-errors:

- scope: SCHEMA
code: CANNOT_DROP_UNKNOWN_COLUMNS
title: Dropped columns are not present in the table schema
title: Columns cannot be dropped if they are not defined in the table schema
Copy link
Contributor

Choose a reason for hiding this comment

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

Could simplify as something like "Cannot drop non-existing columns"

@@ -609,7 +612,7 @@ request-errors:

- scope: SCHEMA
code: CANNOT_VECTORIZE_UNKNOWN_COLUMNS
title: Vectorized columns are not present in the table schema
title: Columns cannot be vectorized if they are not defined in the table schema
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly "Cannot vectorize non-existing columns"?

@@ -618,7 +621,7 @@ request-errors:

- scope: SCHEMA
code: CANNOT_VECTORIZE_NON_VECTOR_COLUMNS
title: Only columns of vector type can be vectorized
title: Columns cannot be vectorized if they are not `vector` type
Copy link
Contributor

Choose a reason for hiding this comment

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

"Cannot vectorize non-vector columns"?

@@ -700,7 +703,7 @@ request-errors:

- scope: SCHEMA
code: UNSUPPORTED_VECTOR_DIMENSION
title: Vector definition contains unsupported configuration
title: Vector definition contains unsupported dimension
body: |-
Copy link
Contributor

Choose a reason for hiding this comment

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

Should similarly clarify full message (configuration -> dimension)

Should this also contain name of column for which problem occurs? (maybe follow-up PR)

@@ -781,7 +783,7 @@ request-errors:

- scope: SCHEMA
code: UNKNOWN_VECTOR_METRIC
title: Vector index creation with unknown vector metric
title: Vector metric is unknown
body: |-
The command attempted to create an vector index using a metric that is not known by the API.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as with most messages: should include name of unknown metric as template.
(follow-up pr).

still some error body could be better.

best or the time we have
gets this error

Caused by: while scanning for the next token
found character '`' that cannot start any token. (Do not use ` for indentation)
@amorton amorton merged commit 8cfdca9 into main Nov 14, 2024
3 checks passed
@amorton amorton deleted the ajm/fix-1662-error-msg-review branch November 14, 2024 02:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Review all errors in errors.yaml
3 participants