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

.columns_dict doesn't work for all possible column types #92

Closed
simonw opened this issue Mar 14, 2020 · 7 comments
Closed

.columns_dict doesn't work for all possible column types #92

simonw opened this issue Mar 14, 2020 · 7 comments
Labels
bug Something isn't working

Comments

@simonw
Copy link
Owner

simonw commented Mar 14, 2020

Got this error:

  File ".../python3.7/site-packages/sqlite_utils/db.py", line 462, in <dictcomp>
    for column in self.columns
KeyError: 'REAL'

.columns_dict uses REVERSE_COLUMN_TYPE_MAPPING:

@property
def columns_dict(self):
"Returns {column: python-type} dictionary"
return {
column.name: REVERSE_COLUMN_TYPE_MAPPING[column.type]
for column in self.columns
}

REVERSE_COLUMN_TYPE_MAPPING defines FLOAT not REALA
REVERSE_COLUMN_TYPE_MAPPING = {
"": str, # Columns in views sometimes have type = ''
"TEXT": str,
"BLOB": bytes,
"INTEGER": int,
"FLOAT": float,
}

@simonw simonw added the bug Something isn't working label Mar 14, 2020
@simonw
Copy link
Owner Author

simonw commented Mar 14, 2020

From https://www.sqlite.org/datatype3.html it looks like FLOAT is a supported keyword for creating tables but REAL is the correct keyword.

So actually sqlite-utils gets this wrong, because when we create a table we turn Python float values into a FLOAT column. Looks like the correct behaviour would be to turn them into a REAL column.

COLUMN_TYPE_MAPPING = {
float: "FLOAT",
int: "INTEGER",
bool: "INTEGER",
str: "TEXT",
bytes.__class__: "BLOB",
bytes: "BLOB",
datetime.datetime: "TEXT",
datetime.date: "TEXT",
datetime.time: "TEXT",
None.__class__: "TEXT",
# SQLite explicit types
"TEXT": "TEXT",
"INTEGER": "INTEGER",
"FLOAT": "FLOAT",
"BLOB": "BLOB",
"text": "TEXT",
"integer": "INTEGER",
"float": "FLOAT",
"blob": "BLOB",
}

@simonw
Copy link
Owner Author

simonw commented Mar 14, 2020

Fixing that would technically constitute a breaking change for library consumers, so it should be a major version release.

I'm not inclined to release 3.0 just for this one issue, so I'm going to hold back on fixing that and address the smaller issue in this bug as a dot release instead for the moment.

@simonw
Copy link
Owner Author

simonw commented Mar 14, 2020

Turns out there are a TON of valid column definitions that aren't being considered yet - https://www.sqlite.org/datatype3.html#affinity_name_examples - stuff like VARYING CHARACTER(255) and DECIMAL(10,5).

@simonw
Copy link
Owner Author

simonw commented Mar 14, 2020

Actually it looks like I should implement the exact rules described in https://www.sqlite.org/datatype3.html#determination_of_column_affinity

The affinity of a column is determined by the declared type of the column, according to the following rules in the order shown:

  1. If the declared type contains the string "INT" then it is assigned INTEGER affinity.
  2. If the declared type of the column contains any of the strings "CHAR", "CLOB", or "TEXT" then that column has TEXT affinity. Notice that the type VARCHAR contains the string "CHAR" and is thus assigned TEXT affinity.
  3. If the declared type for a column contains the string "BLOB" or if no type is specified then the column has affinity BLOB.
  4. If the declared type for a column contains any of the strings "REAL", "FLOA", or "DOUB" then the column has REAL affinity.
  5. Otherwise, the affinity is NUMERIC.

Note that the order of the rules for determining column affinity is important. A column whose declared type is "CHARINT" will match both rules 1 and 2 but the first rule takes precedence and so the column affinity will be INTEGER.

@simonw
Copy link
Owner Author

simonw commented Mar 14, 2020

If the declared type for a column contains the string "BLOB" or if no type is specified then the column has affinity BLOB

I currently treat those as str - it sounds like I should treat them as bytes:

REVERSE_COLUMN_TYPE_MAPPING = {
"": str, # Columns in views sometimes have type = ''

@simonw
Copy link
Owner Author

simonw commented Mar 14, 2020

I'm going to keep treating them as str.

@simonw simonw closed this as completed in 1125460 Mar 14, 2020
@simonw simonw changed the title .columns_dict fails for REAL columns .columns_dict doesn't work for all possible column types Mar 14, 2020
@simonw
Copy link
Owner Author

simonw commented Mar 15, 2020

Released in 2.4.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant