-
Notifications
You must be signed in to change notification settings - Fork 114
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
enh(notion) change how we process dbs #4173
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great but the prefix logic seems not adequate? We want the headler here and not in the csvForDocument
dataSourceConfig, | ||
documentId: `notion-database-${databaseId}`, | ||
documentContent: { | ||
prefix: `${databaseName}\ncsvHeader`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
csvHeader?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is the header right ? What you don't want is DB title ?
happy to remove csv header from the body though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I see it ! Fixed (removed DB name from prefix also, did we want it ?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
used the renderPrefixSection with the defaults -- should I override to have more than 64 tokens for the header ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok ended up doubling it.
df61068
to
de20391
Compare
e8b4748
to
b73c4fc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
dataSourceConfig, | ||
documentId: `notion-database-${databaseId}`, | ||
documentContent: { | ||
prefix: csvHeader, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thiiiink we want the DB name in the prefix no?
Also if csvHeader is > than half chunk size the upsert will fail.
You can likely use the functions that have been built by @philipperolet that are meant to do that but maybe the numbers are a bit tight for this use case.
In any case we want to tokenize and split at half chunk size aka 256 tokens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is outdated right?
Description
Child Database ${DB_TITLE}
) for databases that are children of a blockRisk
Notion sync could get stuck, but easy to rollback
Deploy Plan
Requires a queue bump + restart of workflows