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

proposal: TableCell #3875

Merged
merged 13 commits into from
Feb 1, 2023
Merged

proposal: TableCell #3875

merged 13 commits into from
Feb 1, 2023

Conversation

sjrl
Copy link
Contributor

@sjrl sjrl commented Jan 17, 2023

Proposed Changes:

Adding proposal for the addition of a TableCell primitive/dataclass.

Checklist

@sjrl sjrl requested a review from a team as a code owner January 17, 2023 14:16
@sjrl sjrl requested review from julian-risch, bglearning and ju-gu and removed request for a team January 17, 2023 14:16
proposals/text/3875-table-span.md Outdated Show resolved Hide resolved
proposals/text/3875-table-span.md Outdated Show resolved Hide resolved
proposals/text/3875-table-span.md Outdated Show resolved Hide resolved
proposals/text/3875-table-span.md Outdated Show resolved Hide resolved
proposals/text/3875-table-span.md Outdated Show resolved Hide resolved
proposals/text/3875-table-span.md Outdated Show resolved Hide resolved
@sjrl sjrl changed the title proposal: TableSpan proposal: TableCell Jan 24, 2023
proposals/text/3875-table-span.md Outdated Show resolved Hide resolved
proposals/text/3875-table-span.md Outdated Show resolved Hide resolved
…point to make it clear that we will be able to return a List of TableCells
Copy link
Member

@julian-risch julian-risch 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. 👍 Let's keep the scope of the implementation of this as small as possible and address related issues (Answer/Document subclassing and content_type) separately.

@julian-risch
Copy link
Member

@sjrl As per our proposal process you need approval from two more reviewers before merging. In addition to @ZanSara , either @ju-gu or @bglearning might be able to do that or you need to assign one more reviewer. 🙂

Copy link
Contributor

@bglearning bglearning 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! Will definitely improve TableQA experience for users.

The switch to List[List] from DataFrame for table content representation also seems good for consistency.

(Further discussion on the internal format of tables in haystack Documents can be done at a later time. #3999 )

@sjrl sjrl added the proposal:review Proposal is in "Review" state label Jan 31, 2023
Copy link
Member

@ju-gu ju-gu left a comment

Choose a reason for hiding this comment

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

lgtm!

@sjrl sjrl merged commit 96706e9 into main Feb 1, 2023
@sjrl sjrl deleted the proposal-table-span branch February 1, 2023 08:08
@sjrl sjrl added proposal:active Proposal is in "Active" state and removed proposal:review Proposal is in "Review" state labels Feb 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal:active Proposal is in "Active" state proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants