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

Add a minimal decimal capsule API #85496

Closed
skrah mannequin opened this issue Jul 17, 2020 · 12 comments
Closed

Add a minimal decimal capsule API #85496

skrah mannequin opened this issue Jul 17, 2020 · 12 comments
Labels
3.10 only security fixes topic-C-API type-feature A feature request or enhancement

Comments

@skrah
Copy link
Mannequin

skrah mannequin commented Jul 17, 2020

BPO 41324
Nosy @rhettinger, @mdickinson, @pitrou, @dvarrazzo, @skrah, @mattip
PRs
  • bpo-41324 Add a minimal decimal capsule API #21519
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2020-08-10.14:34:29.785>
    created_at = <Date 2020-07-17.10:55:47.337>
    labels = ['expert-C-API', 'type-feature', '3.10']
    title = 'Add a minimal decimal capsule API'
    updated_at = <Date 2021-05-10.13:51:05.722>
    user = 'https://github.com/skrah'

    bugs.python.org fields:

    activity = <Date 2021-05-10.13:51:05.722>
    actor = 'piro'
    assignee = 'none'
    closed = True
    closed_date = <Date 2020-08-10.14:34:29.785>
    closer = 'skrah'
    components = ['C API']
    creation = <Date 2020-07-17.10:55:47.337>
    creator = 'skrah'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 41324
    keywords = ['patch']
    message_count = 12.0
    messages = ['373823', '373829', '373831', '373842', '373910', '374012', '375121', '388134', '388233', '389244', '393395', '393396']
    nosy_count = 6.0
    nosy_names = ['rhettinger', 'mark.dickinson', 'pitrou', 'piro', 'skrah', 'mattip']
    pr_nums = ['21519']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue41324'
    versions = ['Python 3.10']

    @skrah skrah mannequin added the extension-modules C modules in the Modules dir label Jul 17, 2020
    @skrah skrah mannequin self-assigned this Jul 17, 2020
    @skrah skrah mannequin added 3.10 only security fixes extension-modules C modules in the Modules dir labels Jul 17, 2020
    @skrah skrah mannequin self-assigned this Jul 17, 2020
    @skrah skrah mannequin added the 3.10 only security fixes label Jul 17, 2020
    @skrah
    Copy link
    Mannequin Author

    skrah mannequin commented Jul 17, 2020

    This adds a minimal decimal capsule API. As can be seen from the patch,
    adding anything to decimal while doing it properly is quite labor and
    testing intensive, so the intent is to *keep* the API minimal!

    That said, some functions are really necessary:

    1. PyDec_TypeCheck() -- for obvious reasons.

    2. PyDec_Alloc() -- create new decimals.

    3. PyDec_Get() -- get the mpd_t, enables the use of all libmpdec functions.

    4. PyDec_AsUint128Triple() -- export the decimal as (sign, hi, lo, exp).

    5. PyDec_FromUint128Triple() -- create a decimal from (sign, hi, lo, exp).

    6. and 5) have been requested by Antoine for a real world use case. (hi, lo)
      is the coefficient as a __uint128_t split in two uint64_t.

    Antoine, could you verify that this is sufficient for the database use case?

    @skrah skrah mannequin added type-feature A feature request or enhancement labels Jul 17, 2020
    @skrah
    Copy link
    Mannequin Author

    skrah mannequin commented Jul 17, 2020

    Also as a note for Mark and Raymond: This API is for exact conversions
    and avoids the use of the context except for raising ConversionSyntax.

    I'll add documentation once Antoine has tried it out for his use case.

    @skrah
    Copy link
    Mannequin Author

    skrah mannequin commented Jul 17, 2020

    Adding Daniele Varrazzo, in case this is useful for the PostgreSQL
    adapter.

    @pitrou
    Copy link
    Member

    pitrou commented Jul 17, 2020

    I probably won't try it out explicitly (it's basically cumbersome for us to test with non-release Pythons, because of our dependencies to Numpy and Cython), but reviewing the API should be enough anyway.

    @skrah
    Copy link
    Mannequin Author

    skrah mannequin commented Jul 18, 2020

    It looks like the API would be usable, so the PR now has documentation.

    @Abdulkadirzbudak Abdulkadirzbudak mannequin changed the title Add a minimal decimal capsule API En az ondalık kapsül API ekleme Jul 19, 2020
    @Abdulkadirzbudak Abdulkadirzbudak mannequin changed the title Add a minimal decimal capsule API En az ondalık kapsül API ekleme Jul 19, 2020
    @tirkarthi tirkarthi changed the title En az ondalık kapsül API ekleme Add a minimal decimal capsule API Jul 19, 2020
    @tirkarthi tirkarthi changed the title En az ondalık kapsül API ekleme Add a minimal decimal capsule API Jul 19, 2020
    @skrah
    Copy link
    Mannequin Author

    skrah mannequin commented Jul 20, 2020

    I'm happy with the API, except that --with-system-libmpdec is naturally
    broken. So I've to release a new libmpdec, which I'd rather do soon
    because I don't want to revisit this later.

    Antoine has looked at the API. If anyone else has requests or objections,
    now would be a good time to raise concerns. These two functions are
    the centerpiece of the PR:

    mpd_uint128_triple_t PyDec_AsUint128Triple(const PyObject *dec)
    PyObject *PyDec_FromUint128Triple(const mpd_uint128_triple_t *triple)

    @skrah
    Copy link
    Mannequin Author

    skrah mannequin commented Aug 10, 2020

    New changeset 39042e0 by Stefan Krah in branch 'master':
    bpo-41324 Add a minimal decimal capsule API (bpo-21519)
    39042e0

    @skrah skrah mannequin added topic-C-API and removed extension-modules C modules in the Modules dir labels Aug 10, 2020
    @skrah skrah mannequin closed this as completed Aug 10, 2020
    @skrah skrah mannequin added topic-C-API and removed extension-modules C modules in the Modules dir labels Aug 10, 2020
    @skrah skrah mannequin closed this as completed Aug 10, 2020
    @mattip
    Copy link
    Contributor

    mattip commented Mar 4, 2021

    Was expanding the C-API discussed on the mailing list or in any wider forum? It seems vstinner is working hard to reduce the public API exposure while this issue + PR makes it even larger. Please reconsider putting this in for 3.10.

    @pitrou
    Copy link
    Member

    pitrou commented Mar 7, 2021

    @mattip, see bpo-43422

    @pitrou pitrou unassigned skrah Mar 7, 2021
    @pitrou
    Copy link
    Member

    pitrou commented Mar 21, 2021

    For the record, this is going to be reverted at Stefan's request in bpo-43422.

    @dvarrazzo
    Copy link
    Mannequin

    dvarrazzo mannequin commented May 10, 2021

    FYI, will try to use this API in psycopg3, which supports the PostgreSQL decimal binary format. Thank you very much: it seems exactly what needed.

    Binary conversion with Python Decimal currently has disappointing performances and is slower than the text format conversion. This is likely caused by the use of Decimal.as_tuple(), which creates tuples for the return value and the digits and requires Python calls to access their values.

    I have considered using libmpdec directly: personally I wouldn't be opposed to that, but the additional dependency on libmpdec-dev would earn the project no new fan :\

    Ref. psycopg/psycopg#54

    @dvarrazzo
    Copy link
    Mannequin

    dvarrazzo mannequin commented May 10, 2021

    Ah, just noticed that this has been now reverted. Oh well, I'll see what to do then :(

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.10 only security fixes topic-C-API type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants