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

Following `diesel` sqlite example fails with 1.0.0-beta1 #1392

Closed
bitemyapp opened this Issue Dec 14, 2017 · 29 comments

Comments

Projects
None yet
2 participants
@bitemyapp

bitemyapp commented Dec 14, 2017

   Compiling diesel v1.0.0-beta1 (https://github.com/diesel-rs/diesel?rev=821c840d0c8dccd37ee5f3554304d16c7064ea0a#821c840d)
   Compiling diesel v1.0.0-beta1
   Compiling native-tls v0.1.4
   Compiling tokio-tls v0.1.3
   Compiling hyper-tls v0.1.2
   Compiling egg-mode v0.11.0
   Compiling infer_schema_internals v1.0.0-beta1
   Compiling infer_schema_macros v1.0.0-beta1
   Compiling diesel_infer_schema v1.0.0-beta1 (https://github.com/diesel-rs/diesel?rev=821c840d0c8dccd37ee5f3554304d16c7064ea0a#821c840d)
   Compiling twook v0.1.0 (file:///home/callen/work/twook)
error: unexpected end of macro invocation
  --> src/main.rs:18:5
   |
18 |     infer_schema!("dotenv:DATABASE_URL");
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: this error originates in a macro outside of the current crate

error: aborting due to previous error

error: Could not compile `twook`.

To learn more, run the command again with --verbose.
Makefile:9: recipe for target 'build' failed
make: *** [build] Error 101

Expanded macro invocation error

 -Z external-macro-backtrace`
error: unexpected end of macro invocation
 --> <infer_schema macros>:3:14
  |
1 | | ( $ database_url : expr ) => {
  | |____________________________________^
2 |   mod __diesel_infer_schema {
3 |   # [ derive ( InferSchema ) ] # [
  |  ______________^
src/main.rs:18:5: 18:42 note: in this expansion of infer_schema! (defined in <infer_schema macros>)
<infer_schema macros>:3:14: 3:25 note: in this expansion of #[derive(InferSchema)]

error: aborting due to previous error

The following should be a minimal repro from my main.rs:

#[macro_use]
pub extern crate diesel;
#[macro_use]
extern crate diesel_infer_schema;
#[macro_use]
extern crate dotenv;

mod common;

use common::tokio_core::reactor;
use egg_mode::tweet::Tweet;
use diesel::prelude::*;
use dotenv::dotenv;
use std::env;

infer_schema!("dotenv:DATABASE_URL");
[package]
name = "twook"
version = "0.1.0"

[dependencies]
chrono = "0.4.0"
egg-mode = "0.11.0"
diesel = { git = "https://github.com/diesel-rs/diesel", rev = "821c840d0c8dccd37ee5f3554304d16c7064ea0a", features = ["sqlite"] }
diesel_infer_schema = { git = "https://github.com/diesel-rs/diesel", rev = "821c840d0c8dccd37ee5f3554304d16c7064ea0a", features = ["sqlite"] }
dotenv = "0.9.0"
futures = "*"
tokio-core = "0.1.10"
@bitemyapp

This comment has been minimized.

bitemyapp commented Dec 14, 2017

I've tried stable and nightly, same result.

@sgrif

This comment has been minimized.

Member

sgrif commented Dec 14, 2017

This is happening because you are running infer_schema! on an empty database. You need to run your migrations with diesel migration run as the guide says. I've opened #1393 to fix the error when an empty database is used.

@sgrif sgrif closed this Dec 14, 2017

@bitemyapp

This comment has been minimized.

bitemyapp commented Dec 14, 2017

No, I have migrations and I ran them.

CREATE TABLE tweets (
  id INTEGER PRIMARY KEY,
  coordinates_x REAL,
  coordinates_y REAL,
  created_at INTEGER NOT NULL,
  current_user_retweet INTEGER,
  -- display_text_range_start
  -- display_text_range_end
  tweet_id INTEGER NOT NULL,
  in_reply_to_status_id INTEGER,
  text TEXT NOT NULL,
  user TEXT
)

Re-running diesel migration run yields:

$ diesel migration run                         
[ callen@chalcis ~/work/twook master ✗ ]
$ 

because I'd run them earlier.

@bitemyapp

This comment has been minimized.

bitemyapp commented Dec 14, 2017

On a lark, I tried setting the DATABASE_URL to nonsense and got the same result,

$ DATABASE_URL="blah" make build-verbose

So I think it was looking in the wrong file location, pretty surprising to get this sort of error for that mistake.

New error now is:

error: proc-macro derive panicked
 --> <infer_table_from_schema macros>:2:14
  |
2 | # [ derive ( InferTableFromSchema ) ] # [
  |              ^^^^^^^^^^^^^^^^^^^^
src/main.rs:18:1: 18:38 note: in this expansion of infer_schema! (defined in <infer_schema macros>)
<infer_schema macros>:3:14: 3:25 note: in this expansion of #[derive(InferSchema)]
<infer_schema macros>:3:14: 3:25 note: in this expansion of infer_table_from_schema! (defined in <infer_table_from_schema macros>)
<infer_table_from_schema macros>:2:14: 2:34 note: in this expansion of #[derive(InferTableFromSchema)]
  |
  = help: message: Could not load credentials from database `twook.db`: StringError("Unsupported type: serial")

error: Could not compile `twook`.
@bitemyapp

This comment has been minimized.

bitemyapp commented Dec 14, 2017

Schema in the sqlite3 database:

sqlite> .schema tweets
CREATE TABLE tweets (
  id INTEGER PRIMARY KEY,
  coordinates_x REAL,
  coordinates_y REAL,
  created_at INTEGER NOT NULL,
  current_user_retweet INTEGER,
  -- display_text_range_start
  -- display_text_range_end
  tweet_id INTEGER NOT NULL,
  in_reply_to_status_id INTEGER,
  text TEXT NOT NULL,
  user TEXT
);
@bitemyapp

This comment has been minimized.

bitemyapp commented Dec 14, 2017

Fixed an erroneous (earlier) migration that included SERIAL to be INTEGER, then got this:

error: recursion limit reached while expanding the macro `table_body`
  --> src/main.rs:18:1
   |
18 | infer_schema!("dotenv:DATABASE_URL");
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = help: consider adding a `#![recursion_limit="128"]` attribute to your crate
   = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)
@bitemyapp

This comment has been minimized.

bitemyapp commented Dec 14, 2017

Added recursion limit 128, then got:

error[E0277]: the trait bound `f64: std::hash::Hash` is not satisfied
  --> src/main.rs:73:5
   |
73 |     pub coordinates: Option<(f64, f64)>,
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `std::hash::Hash` is not implemented for `f64`
   |
   = note: required because of the requirements on the impl of `std::hash::Hash` for `(f64, f64)`
   = note: required because of the requirements on the impl of `std::hash::Hash` for `std::option::Option<(f64, f64)>`
   = note: required by `std::hash::Hash::hash`

error[E0277]: the trait bound `f64: std::cmp::Eq` is not satisfied
  --> src/main.rs:73:5
   |
73 |     pub coordinates: Option<(f64, f64)>,
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `std::cmp::Eq` is not implemented for `f64`
   |
   = note: required because of the requirements on the impl of `std::cmp::Eq` for `(f64, f64)`
   = note: required because of the requirements on the impl of `std::cmp::Eq` for `std::option::Option<(f64, f64)>`
   = note: required by `std::cmp::AssertParamIsEq`

error[E0277]: the trait bound `&NewTweet: diesel::Insertable<__diesel_infer_schema::infer_tweets::tweets::table>` is not satisfied
   --> src/main.rs:142:10
    |
142 |         .values(&new_tweet)
    |          ^^^^^^ the trait `diesel::Insertable<__diesel_infer_schema::infer_tweets::tweets::table>` is not implemented for `&NewTweet`

error[E0275]: overflow evaluating the requirement `<_ as diesel::Column>::Table`
   --> src/main.rs:143:10
    |
143 |         .execute(conn)
    |          ^^^^^^^
    |
    = help: consider adding a `#![recursion_limit="256"]` attribute to your crate
@bitemyapp

This comment has been minimized.

bitemyapp commented Dec 14, 2017

After adding recursion limit 256 it seems like my problems are now narrowed down to f64 not being Eq, so I think I'm good to go now.

@sgrif

This comment has been minimized.

Member

sgrif commented Dec 14, 2017

@bitemyapp Did you figure out all your issues related to Diesel? The float problem doesn't seem like it's Diesel related.

@bitemyapp

This comment has been minimized.

bitemyapp commented Dec 14, 2017

@sgrif almost:

#[derive(Queryable)]
#[derive(Debug, Hash, PartialEq)]
pub struct DbTweet {
    pub id: i32,
    pub coordinates_x: Option<Double>,
    pub coordinates_y: Option<Double>,
    pub created_at: DateTime<Utc>,
    pub current_user_retweet: Option<i32>,
    pub tweet_id: i32,
    pub in_reply_to_status_id: Option<i32>,
    pub text: String,
    pub user: Option<String>,
}

#[derive(Insertable)]
#[table_name="tweets"]
pub struct NewTweet {
    pub coordinates_x: Option<Double>,
    pub coordinates_y: Option<Double>,
    pub created_at: DateTime<Utc>,
    pub current_user_retweet: Option<i32>,
    pub tweet_id: i32,
    pub in_reply_to_status_id: Option<i32>,
    pub text: String,
    pub user: Option<String>,
}

Results in the following errors:

`^^^^^^^^^^ the trait `diesel::Expression` is not implemented for `diesel::types::Double`

This seems wrong. I'm trying to harmonize my model struct with the REAL column in my schema. The documentation said to use Double with sqlite as well. What did I do wrong?

Then for a similar but different error:

86 | #[derive(Insertable)]
   |          ^^^^^^^^^^ the trait `diesel::Expression` is not implemented for `chrono::DateTime<chrono::Utc>`

Do I:

  1. Fork Chrono, add diesel as a dependency and add this trait in Chrono to the datatype
  2. Fork Diesel, add a Chrono dependency, add the trait instance to the trait module
  3. Newtype DateTime, reimplement the diesel instances (does Rust have generic newtype deriving like Haskell?), then add the Expression instance for that newtype?
@sgrif

This comment has been minimized.

Member

sgrif commented Dec 14, 2017

I'm not sure I understand what this has to do with chrono. If your column is REAL, that maps to f64 in Diesel.

@bitemyapp

This comment has been minimized.

bitemyapp commented Dec 14, 2017

I think I missed: https://docs.diesel.rs/diesel/types/struct.Timestamp.html but I suspect if I try to put that in my model type I'll run into the same problem I'm experiencing with the floating point type.

Changing the types from Double to f64 netted me this, which is why I went spelunking in the docs, found Double, tried that.

86 | #[derive(Insertable)]
   |          ^^^^^^^^^^ the trait `diesel::Expression` is not implemented for `f64`

Edit:

I tried Timestamp and chrono::NaiveDateTime, got roughly this error for both:

87 | #[derive(Insertable)]
   |          ^^^^^^^^^^ the trait `diesel::Expression` is not implemented for `chrono::NaiveDateTime`
@sgrif

This comment has been minimized.

Member

sgrif commented Dec 14, 2017

Can you please provide the code that is causing your problems, along with the full compiler output? Otherwise I can't do more beyond guess at what you're trying to do, or what's going wrong.

If you want to use chrono::NaiveDateTime, you should call your type either DATETIME or TIMESTAMP.

Glancing through our type mappings in SQLite, it looks like REAL maps to f32 not f64 in 1.0.0-beta1 (that seems like a bug to me). The type names that we map to diesel::types::Double in 1.0.0-beta1 are any types that contain the string "DOUBLE", "DEC", or "NUM"

The error message "Expression is not implemented" is almost always useless. If you look at the full error message, you will see a line that says "required because of the requirements for AsExpression<SqlType> for RustType". That line is the one you want to care about. SqlType is the type we're expecting, and it means that RustType can't be used for that SqlType (typically because it's an unsupported conversion, in the case of f64 in a Float column, or because a feature is missing, which is a common mistake with chrono types).

@sgrif

This comment has been minimized.

Member

sgrif commented Dec 14, 2017

Also you can always see what types got inferred by using diesel print-schema

@sgrif

This comment has been minimized.

Member

sgrif commented Dec 14, 2017

Ah, I see why we map REAL to f32. It's a 32-bit float in ANSI SQL. Also you said you were using that type because our documentation told you to do so. Can you point me to where that was? I can't find that type name mentioned anywhere in our documentation.

@bitemyapp

This comment has been minimized.

bitemyapp commented Dec 14, 2017

I had to use i32 and f32 in the model types to get those to pass. That did seem wrong to me earlier, but using a signed int for what should be an unsigned database id seems wrong to me too. SQLite's Integer can do up to 8 bytes. Are you intentionally restricting it to ANSI rather than what SQLite can do?

I didn't say I was using f64 because of the docs, I said I was trying to use Double in my model type because of the docs. I was in whack-a-mole mode.

The last one appeared to be SQLite's somewhat unfortunate handling of dates requiring that I use Integer for the column type, meaning i32 for the model type.

@bitemyapp

This comment has been minimized.

bitemyapp commented Dec 14, 2017

For posterity's sake, here are the final models and migration (schema):

#[derive(Queryable)]
#[derive(Debug, PartialEq)]
pub struct DbTweet {
    pub id: i32,
    pub coordinates_x: Option<f32>,
    pub coordinates_y: Option<f32>,
    pub created_at: i32,
    pub current_user_retweet: Option<i32>,
    pub tweet_id: i32,
    pub in_reply_to_status_id: Option<i32>,
    pub text: String,
    pub user: Option<String>,
}

#[derive(Insertable)]
#[table_name="tweets"]
pub struct NewTweet {
    pub coordinates_x: Option<f32>,
    pub coordinates_y: Option<f32>,
    pub created_at: i32,
    pub current_user_retweet: Option<i32>,
    pub tweet_id: i32,
    pub in_reply_to_status_id: Option<i32>,
    pub text: String,
    pub user: Option<String>,
}
CREATE TABLE tweets (
  id INTEGER PRIMARY KEY,
  coordinates_x REAL,
  coordinates_y REAL,
  created_at INTEGER NOT NULL,
  current_user_retweet INTEGER,
  -- display_text_range_start
  -- display_text_range_end
  tweet_id INTEGER NOT NULL,
  in_reply_to_status_id INTEGER,
  text TEXT NOT NULL,
  user TEXT
)
@sgrif

This comment has been minimized.

Member

sgrif commented Dec 14, 2017

for what should be an unsigned database id

If you'd like to submit a PR that makes IDs unsigned on all the backends we support, I'd be happy to make that change. However, as it stands right now, they are signed.

Are you intentionally restricting it to ANSI rather than what SQLite can do?

We aren't restricting at all. infer_schema! maps between type names and Diesel's representation of types. We use the type name to figure out what you intended in ways that don't map to SQLite's storage classes. You can safely declare a table as some_col -> BigInt regardless of the type name. (You can use diesel print-schema to get what infer_schema! would have generated, and then edit it as you'd like).

I didn't say I was using f64 because of the docs, I said I was trying to use Double in my model type because of the docs. I was in whack-a-mole mode.

Can you be more specific about what you read that implied that you should do that? I'd like to improve the documentation to fix that error. The module for that type says (emphasis added):

Types which represent a native SQL data type, and the conversions between them and Rust primitives. The structs in this module are only used as markers to represent a SQL type, and shouldn't be used in your structs. See the documentation for each type to see the Rust types that can be used with a corresponding SQL type. Additional types can be added by other crates.

The last one appeared to be SQLite's somewhat unfortunate handling of dates requiring that I use Integer for the column type, meaning i32 for the model type.

SQLite does not have any requirement that you use Integer to store dates

@bitemyapp

This comment has been minimized.

bitemyapp commented Dec 14, 2017

If you'd like to submit a PR that makes IDs unsigned on all the backends we support, I'd be happy to make that change. However, as it stands right now, they are signed.

OK

SQLite does not have any requirement that you use Integer to store dates

I mean, you can also use TEXT or REAL too but that's not much of a choice.

SQLite does not support built-in date and/or time storage class. Instead, it leverages some built-in date and time functions to use other storage classes such as TEXT, REAL, or INTEGER for storing the date and time values.

My point is it's not a real date or timestamp column type.

Can you be more specific about what you read that implied that you should do that? I'd like to improve the documentation to fix that error.

I'm more exhausted now than I was when I was reading it. Maybe another time.

@sgrif

This comment has been minimized.

Member

sgrif commented Dec 14, 2017

I mean, you can also use TEXT or REAL too but that's not much of a choice.

Again, I'm not sure what you're getting at here. SQLite doesn't have a storage class for dates, but that doesn't affect our support for it.

My point is it's not a real date or timestamp column type.

Can you explain to me what you're looking for from a "real" date or timestamp column type that isn't provided?

@bitemyapp

This comment has been minimized.

bitemyapp commented Dec 14, 2017

@sgrif not on your end, SQLite's. Alternative is newtyping and wrapping one of the supported representations.

PostgreSQL has more completely supported date and datetime column types

I double-checked, it turns out, only MySQL has real support for unsigned numbers, save for SQL Server's tinyint, so making the primary keys unsigned, while more faithful to how they get used in practice, is likely to cause some impedance. Unfortunate.

I'm going to see how uniform the support for 64-bit numerics is meanwhile. Should be possible to do better than the ANSI spec.

@sgrif

This comment has been minimized.

Member

sgrif commented Dec 14, 2017

Just to be clear, you can call your column DOUBLE, DECIMAL, or NUMERIC to make infer_schema! infer it as diesel::types::Double, which maps to f64 on the Rust side. (Sorry, our type mappings on SQLite are not well documented at the moment. I'm trying to figure out the best place to document them).

@bitemyapp

This comment has been minimized.

bitemyapp commented Dec 14, 2017

I'll leave it be and just assume something's missing on my end then.

FWIW, when you say:

Just to be clear, you can call your column DOUBLE, DECIMAL, or NUMERIC to make infer_schema!

I have no earthly idea where you're proposing I put that text.

But please don't tell me, put it in the docs if you're motivated. I'll keep-on with keeping-on with my thing rather than trying to fix the size for now.

@sgrif

This comment has been minimized.

Member

sgrif commented Dec 14, 2017

I'm referring to your database schema. e.g. the CREATE TABLE definition.

@bitemyapp

This comment has been minimized.

bitemyapp commented Dec 14, 2017

Integer by itself is guaranteed to support up to 8 bytes by SQLite's documentation.

Your suggested column types don't neatly fit an i64. Numeric can contain any of the five storage classes, so that isn't narrowly or clearly i64. Double is f64, not i64. Decimal falls under Numeric affinity and isn't a 64-bit signed integral number, but rather, a decimal value for which you can specify precision.

The actual column type that maps unambiguously onto i64 in SQLite and is in common use for that purpose is Integer, per the SQLite documentation:

INTEGER. The value is a signed integer, stored in 1, 2, 3, 4, 6, or 8 bytes depending on the magnitude of the value.

@sgrif

This comment has been minimized.

Member

sgrif commented Dec 14, 2017

Your suggested column types don't neatly fit an i64.

I thought we were talking about f64. If you want to use i64 (aka you want infer_schema! to infer your column to be diesel::types::BigInt) you should call your type BIGINT.

Numeric can contain any of the five storage classes, so that isn't narrowly or clearly i64.

I think you may be confused about how SQLite's storage works. SQLite is dynamically typed. Any column can store a value of any type. "Storage classes" apply to values, not columns. Columns are given a "type affinity", which can sometimes affect which storage class is used, but only if that conversion is determined to be lossless. The only time storage classes are ever visible is if you are using CAST, and is generally irrelevant if using Diesel.

Double is f64, not i64

I've never said Double is i64.

Decimal falls under Numeric affinity and isn't a 64-bit whole number, but rather, a Decimal value for which you can specify precision.

The type affinity is virtually meaningless here. It only affects how storage occurs in cases where the conversion is lossless. You should never see any visible change in behavior due to type affinity of a column, and unless you are using CAST, you should never need to care about storage classes. SQLite has no concept of a decimal value for which you can specify precision (aka the decimal type in other backends). The highest precision you can store in SQLite as a numeric value is a 64 bit float. Anything else would need to be stored as a string.

The actual column type that maps unambiguously onto i64 in SQLite and is in common use for that purpose is Integer

As I've mentioned, infer_schema! will put more meaning on your type names than SQLite will. If you call a type SMALLINT, we will infer that to mean you want to work with i16, which is what that type means elsewhere. Same with INTEGER, and BIGINT. This is specific to infer_schema!, not Diesel as a whole. You can safely declare a table to have a column of type BigInt regardless of what you call your type.

If you prefer to avoid infer_schema!s behavior here, I recommend not using it, and using table! directly. You can use diesel print-schema to get the output that infer_schema! generates, and the edit it as you see fit.

@sgrif

This comment has been minimized.

Member

sgrif commented Dec 14, 2017

This bit from the SQLite documentation may help clear things up:

The type affinity of a column is the recommended type for data stored in that column. The important idea here is that the type is recommended, not required. Any column can still store any type of data. It is just that some columns, given the choice, will prefer to use one storage class over another. The preferred storage class for a column is called its "affinity".

@bitemyapp

This comment has been minimized.

bitemyapp commented Dec 14, 2017

I think you may be confused about how SQLite's storage works. SQLite is dynamically typed.

I'm not confused at all. My point is that i64 captures all possible sizes and values of Integer and is the most obvious native representation of SQLite's Integer unless you'd like to have an enum that separates them by size. But that would be silly and I don't think you're suggesting that.

Instead, Diesel infers a type which cannot correctly or validly represent all possible values of an Integer column, i32.

Decimal(5, 2) is definitely a valid column type in SQLite, it just doesn't mean a lot representationally because of SQLite's weird dynamic thing.

I don't think this is steering in a direction that'll do anything useful, but thank you for taking the time so far. Have a good day!

@sgrif

This comment has been minimized.

Member

sgrif commented Dec 14, 2017

If you'd prefer to map to SQLite's type affinities, I recommend avoiding using infer_schema!, and using table! directly instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment