Skip to content

Conversation

@homar
Copy link
Member

@homar homar commented Apr 26, 2023

Description

Docs changes for #16205

Additional context and related issues

Release notes

(x) This is not user-visible or docs only and no release notes are required.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Functions
Table functions

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The connector exposes the following table functions:
The connector provides the following table functions:

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
system.table_changes
table_changes

Copy link
Member

Choose a reason for hiding this comment

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

Wrapping is still weird for this paragraph .. 80 char would be ideal

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
In addition to columns present in the table this function
In addition to columns present in the table

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Type ``VARCHAR``, gives the type of change that occurred,
Gives the type of change that occurred as represented as ``VARCHAR`` value.

Copy link
Member

Choose a reason for hiding this comment

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

add schema_name and table_name as required parameters

Copy link
Member

Choose a reason for hiding this comment

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

I think we should at least move values down into a separate line

Copy link
Member

Choose a reason for hiding this comment

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

three lines

Copy link
Member Author

Choose a reason for hiding this comment

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

sorry i don't get this comment :|

Copy link
Member

Choose a reason for hiding this comment

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

 UPDATE pages 
   SET domain = 'domain4' 
   WHERE views = 2;

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
In the output it can be easily seen what changes happen in which version.
The output shows what changes happen in which version.

Copy link
Member

Choose a reason for hiding this comment

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

From 833 to 861 .. this is all the same as above right. .. if so .. just omit it all

@homar homar force-pushed the homar/table_changes_docs branch from 3f6001f to 0ca3e00 Compare April 27, 2023 08:37
Copy link
Member

Choose a reason for hiding this comment

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

Wrap up

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Gives the type of change that occurred as represented as ``VARCHAR`` value.
Gives the type of change that occurred.

and wrap

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Shows the table version for which the change occurred as ``BIGINT`` value.
Shows the table version for which the change occurred.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
``TIMESTAMP(3) WITH TIME ZONE`` data type value.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Represents the timestamp for the commit in which the specified change happened as
Represents the timestamp for the commit in which the specified change happened.

Copy link
Member

Choose a reason for hiding this comment

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

I think we need to add the schema to the queries above to be clear

Copy link
Member

Choose a reason for hiding this comment

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

Or add a "use" command first

Copy link
Member

Choose a reason for hiding this comment

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

We can omit that .. its implied since we are still talking about the same example

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The preceding sequence of SQL statements returns the following result:
The preceding SQL statement returns the following result:

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Here we can see changes that occurred at version 1 - three inserts - they were
You can see changes that occurred at version 1 as- three inserts. They are

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
not visible in the previous code snippet when since_version value was set to 1.
not visible in the previous statement when since_version value was set to 1.

@mosabua
Copy link
Member

mosabua commented May 1, 2023

I simplified a bit..

@mosabua
Copy link
Member

mosabua commented May 11, 2023

Let me know when you have the code PR ready and this updates so I can review for latest status. Also currently this PR conflicts..

@mosabua
Copy link
Member

mosabua commented May 12, 2023

@findepi merged the code PR. @homar can you please make sure you resolve the conflicts and address any comments as soon as possible so we can make sure we get this PR merged before the release next week. Also adapt the docs to whatever changed in the code before merge ..

@homar homar force-pushed the homar/table_changes_docs branch from 0ca3e00 to 32c4b40 Compare May 14, 2023 09:48
@homar
Copy link
Member Author

homar commented May 14, 2023

@findepi merged the code PR. @homar can you please make sure you resolve the conflicts and address any comments as soon as possible so we can make sure we get this PR merged before the release next week. Also adapt the docs to whatever changed in the code before merge ..

Yeah sorry, I was in warsaw office for 2 days and I didn't have time to update this one. I have updated it only now

@homar homar requested a review from mosabua May 17, 2023 16:39
Copy link
Member

@colebow colebow left a comment

Choose a reason for hiding this comment

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

One nit, but LGTM.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
In addition to columns present in the table the function returns the following
In addition to returning the columns present in the table, the function
returns the following

Copy link
Member

Choose a reason for hiding this comment

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

Please apply this suggestion @homar

Copy link
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

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

Needs some reordering

Copy link
Member

Choose a reason for hiding this comment

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

Expand this to a sentence.

Copy link
Member

Choose a reason for hiding this comment

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

as- ??

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
not visible in the previous statement when since_version value was set to 1.
not visible in the previous statement when ``since_version`` value was set to 1.

@mosabua
Copy link
Member

mosabua commented May 24, 2023

@homar .. are you updating this PR so we can review again and maybe merge soon?

@homar homar force-pushed the homar/table_changes_docs branch from 32c4b40 to f949996 Compare May 24, 2023 09:28
Copy link
Member

@mosabua mosabua 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 now. Let's ship it.

Copy link
Member

@mosabua mosabua 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 now. Let's ship it.

@mosabua mosabua merged commit 144840d into trinodb:master May 25, 2023
@github-actions github-actions bot added this to the 419 milestone May 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

3 participants