GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-24905: Support BLOB incremental I/O in sqlite module by palaviv · Pull Request #271 · python/cpython

Conversation

@palaviv

@palaviv palaviv commented Feb 24, 2017 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

This PR adds support in BLOB incremental I/O at the sqlite module. As asked by @serhiy-storchaka and @berkerpeksag I will try to get some more developers to give their input on the wanted API. I am tagging some people that are active in the ghaering/pysqlite and rogerbinns/apsw.
@ghaering, @rianhunter, @rogerbinns, @phdru. Please look at the PR and give your notes.

https://bugs.python.org/issue24905

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bozhinov, graingert, erlend-aasland, and chrisprobst reacted with thumbs up emoji SomeoneSerge, chrisprobst, and DavidBuchanan314 reacted with heart emoji
All reactions
  • 👍 4 reactions
  • ❤️ 3 reactions
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Feb 24, 2017
@rogerbinns

rogerbinns commented Feb 24, 2017

Copy link
Copy Markdown

The APSW doc for reference is at https://rogerbinns.github.io/apsw/blob.html

Does having len make sense? Files don't have that method. It is also confusing - should len return the size from the current seek offset?

The documentation should make clearer that you cannot change the size of a blob, and mention zeroblob as the means to make a blob in a query without having to fill it in.

It may be worth mentioning that another approach is to store large data in a file, and only store the filename in the database. (This comes up on the sqlite-users mailing list quite a lot.)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@phdru

phdru commented Feb 24, 2017

Copy link
Copy Markdown

I cannot remember I ever was in need to read/write a part of a BLOB; it was always "all or nothing" for me. So I never used BLOB APIs; instead I always SELECT/INSERT/UPDATE BLOB columns; in Postgres they are not even BLOB columns — I always use BYTEA type.

So I'm -0 on exposing BLOB API for SQLite.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@rogerbinns

rogerbinns commented Feb 24, 2017

Copy link
Copy Markdown

@phdru SQLite is the same with regular queries: you can only read or write blobs in their entirety. That for example means that if you store a 25MB blob then you must read or write 25MB at once.

SQLite has the "incremental blob" API for accessing just portions of blobs. The motivation comes from "Lite" in the name - developers use SQLite because it is lighter weight (amongst other reasons). DBAPI doesn't specify incremental blob I/O so only developers intending to use SQLite directly and not another database would use it. Should they be able to?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@phdru

phdru commented Feb 24, 2017

Copy link
Copy Markdown

-0 from me means: I don't care and if there will be such an API I'm not gonna use it. That's all.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@palaviv

palaviv commented Feb 25, 2017

Copy link
Copy Markdown
Contributor Author

Thanks for the input @rogerbinns.

Does having len make sense? Files don't have that method. It is also confusing - should len return the size from the current seek offset?

What is the difference between implementing __len__ to the method length APSW blob has?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@rogerbinns

rogerbinns commented Feb 28, 2017

Copy link
Copy Markdown

@palaviv there is no difference between the value returned by len and length or similar methods. It is however very uncommon to have a len method on file like objects - I couldn't find an example of any! For example StringIO is closest and has no len. Hence my recommendation to avoid len in favour of another method name.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@serhiy-storchaka

serhiy-storchaka commented Feb 28, 2017

Copy link
Copy Markdown
Member

The mmap.mmap() object is an example of file-like object supporting len().

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@rogerbinns

rogerbinns commented Feb 28, 2017

Copy link
Copy Markdown

@serhiy-storchaka good example. They don't document it though, and there is a size() method although it is returning something slightly different. There also seems to be a correlation between types that have len and those that can you can array access.

In any event my recommendation is to avoid breaking new ground with a len method since that seems not to be normal practise for this kind of thing that provides a file like interface.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@palaviv

palaviv commented Feb 28, 2017

Copy link
Copy Markdown
Contributor Author

I actually think that we should use __len__ as by the definition this is the length of the object. The Blob object is a representation of the BLOB and that is the BLOB length.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@serhiy-storchaka

serhiy-storchaka commented Feb 28, 2017

Copy link
Copy Markdown
Member

Pull request conversation is purposed for discussing the code.

It would be better to continue the design discussion on the bug tracker or mailing list.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@palaviv

palaviv commented Mar 4, 2017

Copy link
Copy Markdown
Contributor Author

@serhiy-storchaka I have implemented the sequence protocol but I have a few questions:

  1. Do I need both PySequenceMethods.sq_item, PySequenceMethods.sq_ass_item and PyMappingMethods.mp_subscript, PyMappingMethods.mp_ass_subscript.
  2. I can't make __contains__ work. Could you point me to how fix that?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Mariatta Mariatta added needs rebase and removed needs rebase labels Oct 9, 2017
@brettcannon brettcannon added the awaiting review label Feb 21, 2018
@palaviv
palaviv force-pushed the issue-24905-sqlite-blob branch from 4373ce8 to 24ed220 Compare April 18, 2018 13:04
@palaviv

palaviv commented Apr 18, 2018

Copy link
Copy Markdown
Contributor Author

I think that the contains operation should not be supported for blobs. As blobs can be very large looking for a subset of bytes inside them will be a very inefficient process in memory or in compute.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@palaviv
palaviv requested a review from a team as a code owner April 18, 2018 13:32
auvipy
auvipy reviewed Oct 9, 2018
View reviewed changes
Comment thread Doc/library/sqlite3.rst Outdated
The BLOB size cannot be changed using the :class:`Blob` class. Use
``zeroblob`` to create the blob in the wanted size in advance.

.. versionadded:: 3.7

@auvipy auvipy Oct 9, 2018

Copy link
Copy Markdown

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.

3.8

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@palaviv palaviv May 8, 2019

Copy link
Copy Markdown
Contributor Author

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.

Done

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Doc/library/sqlite3.rst Outdated
Blob Objects
------------

.. versionadded:: 3.7

@auvipy auvipy Oct 9, 2018

Copy link
Copy Markdown

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.

3.8

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@palaviv palaviv May 8, 2019

Copy link
Copy Markdown
Contributor Author

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.

Done

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@auvipy auvipy Sep 17, 2019

Copy link
Copy Markdown

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.

you can retarget this for py 3.9

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
ZackerySpytz
ZackerySpytz reviewed Mar 30, 2019
View reviewed changes
Comment thread Modules/_sqlite/blob.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@matrixise

matrixise commented May 7, 2019

Copy link
Copy Markdown
Member

Hi @palaviv

Would you be interested to upgrade your PR to the last master?

Thank you

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@matrixise
matrixise requested a review from berkerpeksag May 7, 2019 20:47
@palaviv
palaviv force-pushed the issue-24905-sqlite-blob branch 2 times, most recently from efac873 to 765545e Compare May 9, 2019 09:55
auvipy
auvipy suggested changes Sep 17, 2019
View reviewed changes

@auvipy auvipy left a comment

Copy link
Copy Markdown

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.

Retarget for python 3.9

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Doc/library/sqlite3.rst Outdated

@auvipy auvipy Sep 17, 2019

Copy link
Copy Markdown

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.

3.9

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-bot bedevere-bot added awaiting core review and removed awaiting review labels Sep 17, 2019
@eamanu

eamanu commented Jun 21, 2020

Copy link
Copy Markdown
Contributor

Hi @palaviv There is some plan with this PR?
Was open 3 years ago. Are you still interest on this patch?
If yes, could you fix the conflict. please?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@palaviv

palaviv commented Jul 4, 2020

Copy link
Copy Markdown
Contributor Author

Hi @eamanu,
This patch exist since January 2016 and I kind of given up on it ever going into CPython as there is no core developer that works on sqlite. I would recommend you to use apsw that support this feature. In case any core developer would be in interested in working on this I would gladly fix any needed changes.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@simonw

simonw commented Jul 27, 2020

Copy link
Copy Markdown
Contributor

I'd love to see this land in Python. I think there's a strong case for it: SQLite lets you store up to 2GB of data in a BLOB, and reading an entire 2GB value into memory at once isn't nearly as pleasant as reading it incrementally, which is what this would let us do.

saulshanabrook, SomeoneSerge, and DavidBuchanan314 reacted with thumbs up emoji
All reactions
  • 👍 3 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

palaviv added 4 commits August 3, 2020 19:08
@palaviv
Fix CR comments in code
9709456
@palaviv
Make readonly and dbname keyword arguements only
e6e5099
@palaviv
Fix more CR comments
e9a8080
@palaviv
Add more indicative error on write bigger then blob length
d4fb1b5
@palaviv

palaviv commented Aug 3, 2020

Copy link
Copy Markdown
Contributor Author

Thanks for the review @berkerpeksag. I have made the requested changes; please review again.

bozhinov reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot added awaiting change review and removed awaiting changes labels Aug 3, 2020
@bedevere-bot

bedevere-bot commented Aug 3, 2020

Copy link
Copy Markdown

Thanks for making the requested changes!

@berkerpeksag: please review the changes made to this pull request.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot
bedevere-bot requested a review from berkerpeksag August 3, 2020 16:58
@coleifer coleifer mentioned this pull request Oct 23, 2020
Where is zeroblob ? coleifer/pysqlite3#16
Closed
@nightlark

nightlark commented Aug 18, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Other than rebasing (due new conflicts arising over time), is there anything that can be done to help move this PR along?

(@palaviv do you want to do the rebase? if you'd like or are too busy I can do the rebase, though I'd need to open a new PR since I don't think I can modify yours)

erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland

erlend-aasland commented Aug 18, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

@nightlark, @palaviv: Here' a short list from the top-of-my head of what is needed to rebase this onto main:

  • the test suite has been normalized; we now use snake case test_foo_bar method names
  • Argument Clinic
  • use heap types iso. static types
  • exception types are accessed through the (temporary) global state; for Connection objects, it's available through self->state

If you want to try to land this, Ryan, please give Aviv a week or so to respond before opening a new PR :)

nightlark reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nightlark

nightlark commented Aug 20, 2021

Copy link
Copy Markdown
Contributor

@erlend-aasland Okay — I think I understand how to use argument clinic. Is there a guide to what iso. static types (or heap types)? Is the iso. a prefix for the types or an abbreviation? If it’s an abbreviation maybe that’s the missing a search term I should be using to find relevant resources.

erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland

erlend-aasland commented Aug 20, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

@erlend-aasland Okay — I think I understand how to use argument clinic.

Great! AC is nice once you get into it. Feel free to ask if you get stuck :)

Is there a guide to what iso. static types (or heap types)?

There's some info in the docs, but you can also check the PR's that converted the existing types:

  • bpo-41861: Convert _sqlite3 CursorType and ConnectionType to heap types #22478
  • bpo-41861: Convert _sqlite3 RowType and StatementType to heap types #22444
  • bpo-41861: Convert sqlite3 PrepareProtocolType to heap type #22428
  • bpo-41861: Convert sqlite3 cache and node types to heap types #22417
  • bpo-42972: Fully implement GC protocol for sqlite3 heap types #26104, bpo-42972: Fix sqlite3 traverse/clear #26452 (see these PR's for proper GC support)

Don't hesitate to ask if you need more pointers.

Is the iso. a prefix for the types or an abbreviation?

Sorry, it's an abbreviation: instead of :) I have the bad habit of using it too much.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland

erlend-aasland commented Aug 20, 2021

Copy link
Copy Markdown
Contributor

Regarding heap types, take a look at Victor's blog: https://vstinner.github.io/isolate-subinterpreters.html

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland

erlend-aasland commented Oct 19, 2021

Copy link
Copy Markdown
Contributor

@nightlark Are you still working on this PR?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland erlend-aasland mentioned this pull request Jan 3, 2022
gh-69093: Add incremental I/O to blobs support in sqlite3 #30356
Closed
2 tasks
@CoolCat467

CoolCat467 commented Feb 1, 2022

Copy link
Copy Markdown
Contributor

It's 2022 now woooo

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@dholth

dholth commented Feb 16, 2022

Copy link
Copy Markdown
Contributor

+1

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@github-actions

github-actions Bot commented Mar 19, 2022

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Mar 19, 2022
@erlend-aasland erlend-aasland mentioned this pull request Apr 12, 2022
gh-69093: Support basic incremental I/O to blobs in sqlite3 #30680
Merged
erlend-aasland pushed a commit to erlend-aasland/cpython that referenced this pull request Apr 14, 2022
Erlend E. Aasland
Address Berker's comments from pythongh-271: remove unneeded class pr…
2590112
…efix
erlend-aasland pushed a commit to erlend-aasland/cpython that referenced this pull request Apr 14, 2022
Erlend E. Aasland
Address Berker's comments from pythongh-271: move __len__ doc to clas…
69a5a1e
…s description
@JelleZijlstra

JelleZijlstra commented Apr 15, 2022

Copy link
Copy Markdown
Member

I just merged #30680, a simplified version of this PR. Blobs will be in Python 3.11.

saulshanabrook and DavidBuchanan314 reacted with hooray emoji
All reactions
  • 🎉 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@JelleZijlstra JelleZijlstra closed this Apr 15, 2022
@jimminter jimminter mannequin mentioned this pull request Apr 15, 2022
Allow incremental I/O to blobs in sqlite3 #69093
Closed
This was referenced Apr 15, 2022
gh-69093: Add context manager support to sqlite3.Blob #91562
Merged
gh-69093: Add mapping protocol support to sqlite3.Blob #91599
Merged
jaraco pushed a commit that referenced this pull request Dec 2, 2022
@dependabot-preview
Bump aiohttp from 3.5.4 to 3.6.0 (#271)
539e351
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@berkerpeksag berkerpeksag Awaiting requested review from berkerpeksag
2 more reviewers
@ZackerySpytz ZackerySpytz ZackerySpytz left review comments
@auvipy auvipy auvipy requested changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting change review stale Stale PR or inactive for long period of time.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

19 participants

@palaviv @rogerbinns @phdru @serhiy-storchaka @matrixise @eamanu @simonw @berkerpeksag @bedevere-bot @nightlark @erlend-aasland @CoolCat467 @dholth @JelleZijlstra @auvipy @ZackerySpytz @brettcannon @Mariatta @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.