How do I use Room’s prepackagedDatabaseCallback?

So how could I uses this to circumvent the Invalid Schema expected …. found ….?

The following, albeit long winded shows an example that corrects a schema accordingly.

Could I use this to introduce Triggers, as room doesn’t have annotations for the creation of triggers?

Yes, although this hasn’t actually been tested but the example caters for the creation of the triggers.

Notes regrading the example
First, it should be noted that when the callback was initially being looked at that there were issues. These have now been addressed BUT requires 2.4.0-beta02 or greater. The issues were:-

  1. Not allowing version 0 in the pre-packaged database (often this would be the case)
  2. Empty Database passed to PrePackagedDatabaseCallback
  3. Database updates applied in PrePackagedDatabaseCallback Lost/Undone
  • The fix applied being

  • Correctly open pre-package database during PrepackagedDatabaseCallback invocation.

  • The actual pre-package database was not being opened because only the file name was being used when creating the temporary open helper instead of the full path. The framework open helper will only open an existing file if the name of the database is a path (starts with “https://stackoverflow.com/”) otherwise it create a new file. This changes fixes it by using the absolute file path as name.

The Flow

When control is passed to the callback the asset database has been copied. So it’s just a matter, in theory, of ALTERing the TABLES and VIEWS (if any) to match the schema that room expects.

Often trying to match what room expects with what room finds frustrates some. The example below will overcome minor/small/often missed issues. It works by:-

  1. Dropping all non-sqlite or non-android components other than the tables (i.e. views, indexes and triggers from the copied assets database),
  2. Renaming the tables to allow the ones that room expects to be created,
  3. Creating new tables using Room’s expected schema,
    1. as copied from the java that Room generates
  4. Copying the data from the renamed (asset) tables into the newly created tables
    1. INSERT OR IGNORE has been used so constraint conflicts, such as NOT NULL, UNIQUE, CHECK will not result in an exception (change INSERT OR IGNORE to just INSERT to fail)
  5. Creating the other components that Room expects (views, indexes and triggers (note Room only has triggers for FTS tables)).
  6. Dropping the now redundant renamed tables
  7. Doing a VACUUM to clean up the database,
  8. and finally adding any triggers.

It does expect that the source (asset) has the columns in the correct order, that nulls are are not included in columns that room has a NOT NULL constraint. However, as INSERT OR IGNORE is used then such rows will not be inserted rather than resulting in an exception.

Other than copying come code from the generated java, the process is automated and should cope with most/many assets without modification.

The Code

The vast majority of the code is in the @Database class OtherDatabase. Note that this should cope with many databases with a little tailoring (see comments for changes to make):-

@SuppressLint({"Range"}) /*<<<<< used due to bug/issue with getColumnIndex introduced with SDK 31 */
@Database(
        entities = {Person.class, Company.class, CompanyPersonMap.class}, //<<<<< CHANGED ACCORDINGLY
        version = OtherDatabase.DATABASE_VERSION, /* note due to shared usage of DBVERSION OtherDatabase used <<<<< CHANGE ACCORDINGLY */
        exportSchema = false /* change to suit */
)
@TypeConverters({AllTypeConverters.class})
abstract class OtherDatabase extends RoomDatabase {
    public static final String DATABASE_NAME = "otherprepackageddatabasecallbacktest.db"; //<<<<< CHANGE AS REQUIRED
    public static final int DATABASE_VERSION = 1; //<<<<< CHANGE AS REQUIRED
    public static final String ASSET_FILE_NAME = "prepackageddatabasecallbacktest.db"; //<<<<< CHANGED AS REQUIRED
    /**
     *  sqlite_master table and column names !!!!!DO NOT CHANGE!!!!!
     */
    private static final String SQLITE_MASTER_TABLE_NAME = "sqlite_master";
    private static final String SQLITE_MASTER_COL_NAME = "name";
    private static final String SQLITE_MASTER_COL_TYPE = "type";
    private static final String SQLITE_MASTER_COL_TABLE_NAME = "tbl_name";
    private static final String SQLITE_MASTER_COL_SQL = "sql";

    abstract AllDao getAllDao(); //<<<<< CHANGE ACCORDINGLY
    private static volatile OtherDatabase instance = null; //<<<<< CHANGE ACCORDINGLY
    public static OtherDatabase /*<<<< CHANGE ACCORDINGLY */ getInstance(Context context) {
        if (instance == null) {
            instance = Room.databaseBuilder(context, OtherDatabase.class, OtherDatabase.DATABASE_NAME)
                    .allowMainThreadQueries() /*<<<<< USED FOR BREVITY CONVENIENCE */
                    .createFromAsset(ASSET_FILE_NAME, prePkgDbCallback) /* 2nd parameter is the THE CALL BACK to invoke */
                    .build();
        }
        return instance;
    }

    /* THE CALLBACK  */
    static final PrepackagedDatabaseCallback prePkgDbCallback = new PrepackagedDatabaseCallback() {
        final static String assetTablePrefix = "asset_"; /* prefix used for renamed tables - should not need to be changed */
        private List<SQLiteMasterComponent> sqLiteMasterComponentArray; /* store for sqlite_master extract */
        @Override
        public void onOpenPrepackagedDatabase(@NonNull SupportSQLiteDatabase db) {
            super.onOpenPrepackagedDatabase(db);
            sqLiteMasterComponentArray = buildComponentsList(db); /* gets relevant rows from sqlite_master */
            dropNonTableComponents(sqLiteMasterComponentArray,db); /* everything except the tables */
            renameTableComponents(sqLiteMasterComponentArray, assetTablePrefix,db); /* rename the tables using prefix */
            /*<<<<< TAILOR (the db.execSQL's below) AS APPROPRIATE - SEE COMMENTS THAT FOLLOW >>>>>*/
            /* copied from the @Database classes generated java e.g. leave indexes till later
                _db.execSQL("CREATE TABLE IF NOT EXISTS `Person` (`personid` INTEGER, `firstName` TEXT, `lastName` TEXT, `middleNames` TEXT, `dateOfBirth` INTEGER, PRIMARY KEY(`personid`))");
                _db.execSQL("CREATE INDEX IF NOT EXISTS `index_Person_firstName` ON `Person` (`firstName`)");
                _db.execSQL("CREATE INDEX IF NOT EXISTS `index_Person_lastName` ON `Person` (`lastName`)");
                _db.execSQL("CREATE TABLE IF NOT EXISTS `company` (`companyid` INTEGER, `companyName` TEXT, `city` TEXT, `state` TEXT, `country` TEXT, `notes` TEXT, PRIMARY KEY(`companyid`))");
                _db.execSQL("CREATE TABLE IF NOT EXISTS `company_person_map` (`companyid_map` INTEGER NOT NULL, `personid_map` INTEGER NOT NULL, PRIMARY KEY(`companyid_map`, `personid_map`), FOREIGN KEY(`companyid_map`) REFERENCES `company`(`companyid`) ON UPDATE CASCADE ON DELETE CASCADE , FOREIGN KEY(`personid_map`) REFERENCES `Person`(`personid`) ON UPDATE CASCADE ON DELETE CASCADE )");
                _db.execSQL("CREATE INDEX IF NOT EXISTS `index_company_person_map_personid_map` ON `company_person_map` (`personid_map`)");
             */
            /* Create the tables as per Room definitions - ***** CREATE TABLES COPIED FROM GENERATED JAVA *****
                only indexes, views, triggers (for FTS) should be done after the data has been copied so :-
                    data is loaded faster as no index updates are required.
                    triggers don't get triggered when loading the data which could result in unexpected results
             */
            db.execSQL("CREATE TABLE IF NOT EXISTS `Person` (`personid` INTEGER, `firstName` TEXT, `lastName` TEXT, `middleNames` TEXT, `dateOfBirth` INTEGER, PRIMARY KEY(`personid`));");
            db.execSQL("CREATE TABLE IF NOT EXISTS `company` (`companyid` INTEGER, `companyName` TEXT, `city` TEXT, `state` TEXT, `country` TEXT, `notes` TEXT, PRIMARY KEY(`companyid`))");
            db.execSQL("CREATE TABLE IF NOT EXISTS `company_person_map` (`companyid_map` INTEGER NOT NULL, `personid_map` INTEGER NOT NULL, PRIMARY KEY(`companyid_map`, `personid_map`), FOREIGN KEY(`companyid_map`) REFERENCES `company`(`companyid`) ON UPDATE CASCADE ON DELETE CASCADE , FOREIGN KEY(`personid_map`) REFERENCES `Person`(`personid`) ON UPDATE CASCADE ON DELETE CASCADE )");

            copyData(sqLiteMasterComponentArray, assetTablePrefix,db); /* copy the data from the renamed asset tables to the newly created Room tables */

            /* Create the other Room components - ***** CREATE ? COPIED FROM GENERATED JAVA *****
                Now that data has been copied create other Room components indexes, views and triggers
                (triggers would only be for FTS (full text search))
                again sourced from generated Java
             */
            db.execSQL("CREATE INDEX IF NOT EXISTS `index_Person_firstName` ON `Person` (`firstName`)");
            db.execSQL("CREATE INDEX IF NOT EXISTS `index_Person_lastName` ON `Person` (`lastName`)");
            db.execSQL("CREATE INDEX IF NOT EXISTS `index_company_person_map_personid_map` ON `company_person_map` (`personid_map`)");
            dropRenamedTableComponents(sqLiteMasterComponentArray, assetTablePrefix,db); /* done with the renamed tables so drop them */
            db.execSQL("VACUUM"); /* cleanup the database */
            createTriggers(sqLiteMasterComponentArray,db); /* create any triggers */
        }
    };

    static int dropNonTableComponents(List<SQLiteMasterComponent> components, SupportSQLiteDatabase db) {
        int rv = 0;
        for(SQLiteMasterComponent c: components) {
            if (!c.type.equals("table") ) {
                db.execSQL("DROP " +  c.type + " IF EXISTS " + c.name);
                rv++;
            }
        }
        return rv;
    }

    static int dropRenamedTableComponents(List<SQLiteMasterComponent> components, String prefix, SupportSQLiteDatabase db) {
        int rv = 0;
        int maxForeignKeyCount = 0;
        for (SQLiteMasterComponent c: components) {
            if (c.type.equals("table") && c.foreignKeyCount > maxForeignKeyCount) {
                maxForeignKeyCount = c.foreignKeyCount;
            }
        }
        for (int i= maxForeignKeyCount; i >= 0; i--) {
            for (SQLiteMasterComponent c: components) {
                if (c.type.equals("table") && c.foreignKeyCount == i) {
                    db.execSQL("DROP " + c.type + " IF EXISTS " + prefix + c.name);
                    rv++;
                }
            }
        }
        return rv;
    }

    static int renameTableComponents(List<SQLiteMasterComponent> components, String prefix, SupportSQLiteDatabase db) {
        int rv = 0;
        db.execSQL("PRAGMA foreign_keys = ON"); // Just in case turn foreign keys on
        for(SQLiteMasterComponent c: components) {
            if (c.type.equals("table")) {
                db.execSQL("ALTER TABLE " + c.name + " RENAME TO " + prefix + c.name);
                rv++;
            }
        }
        return rv;
    }

    /*
        NOTE tables with fewest Foreign Key definitions done first
        NOTE makes an assumption that this will not result in FK conflicts
        TODO should really be amended to ensure that a table with FK's is only attempted when all of it's parent tables have been loaded
     */
    static int copyData(List<SQLiteMasterComponent> components, String prefix, SupportSQLiteDatabase db) {
        int rv = 0;
        int maxForeignKeyCount = 0;
        for (SQLiteMasterComponent c: components) {
            if (c.type.equals("table") && c.foreignKeyCount > 0) {
                maxForeignKeyCount = c.foreignKeyCount;
            }
        }
        for (int i=0; i <= maxForeignKeyCount; i++) {
            for (SQLiteMasterComponent c: components) {
                if (c.type.equals("table") && c.foreignKeyCount == i) {
                    db.execSQL("INSERT OR IGNORE INTO " + c.name + " SELECT * FROM " + prefix + c.name + ";");
                    rv++;
                }
            }
        }
        return rv;
    }

    static int createTriggers(List<SQLiteMasterComponent> components, SupportSQLiteDatabase db) {
        int rv = 0;
        for (SQLiteMasterComponent c: components) {
            if (c.type.equals("trigger")) {
                //TODO should really check if the sql includes IF NOT EXISTSand if not add IF NOT EXISTS
                db.execSQL(c.sql);
                rv++;
            }
        }
        return rv;
    }

    /**
     * Build the list of required SQLiteMasterComponents to save having to access
     * sqlite_master many times.
     * @param db the SupportSQliteDatabase to access
     * @return the list of SQliteMasterComponents extracted
     */
    @SuppressLint("Range")
    static List<SQLiteMasterComponent> buildComponentsList(SupportSQLiteDatabase db) {
        final String FOREIGN_KEY_FLAG_COLUMN =  "foreign_key_flag";
        ArrayList<SQLiteMasterComponent> rv = new ArrayList<>();
        Cursor csr = db.query("SELECT *," +
                /* Column to indicate wherther or not FK constraints appear to have been defined
                 *  NOTE!! can be fooled
                 *       e.g. if a column is defined as `my  badly named FOREIGN KEY column ` ....
                 * */
                "(" +
                "instr(" + SQLITE_MASTER_COL_SQL + ",'FOREIGN KEY') > 0) + " +
                "(instr(" + SQLITE_MASTER_COL_SQL + ",' REFERENCES ')> 0) " +
                "AS " + FOREIGN_KEY_FLAG_COLUMN + " " +
                "FROM " + SQLITE_MASTER_TABLE_NAME + " " +
                /* do not want any sqlite tables or android tables included */
                "WHERE lower(" + SQLITE_MASTER_COL_NAME + ") NOT LIKE 'sqlite_%' AND lower(" + SQLITE_MASTER_COL_NAME + ") NOT LIKE 'android_%'");
        while (csr.moveToNext()) {
            SQLiteMasterComponent component = new SQLiteMasterComponent(
                    csr.getString(csr.getColumnIndex(SQLITE_MASTER_COL_NAME)),
                    csr.getString(csr.getColumnIndex(SQLITE_MASTER_COL_TYPE)),
                    csr.getString(csr.getColumnIndex(SQLITE_MASTER_COL_TABLE_NAME)),
                    csr.getString(csr.getColumnIndex(SQLITE_MASTER_COL_SQL)),
                    csr.getInt(csr.getColumnIndex(FOREIGN_KEY_FLAG_COLUMN)),
                    0);
            if (csr.getInt(csr.getColumnIndex(FOREIGN_KEY_FLAG_COLUMN)) > 0) {
                component.foreignKeyCount = component.getForeignKeyCount(db);
            }
            rv.add(component);
        }
        csr.close();
        return (List<SQLiteMasterComponent>) rv;
    }

    /**
     *  Class to hold a row from sqlite_master
     */
    private static class SQLiteMasterComponent {
        private String name;
        private String type;
        private String owningTable;
        private String sql;
        private int foreignKeyFlag = 0;
        private int foreignKeyCount = 0;

        SQLiteMasterComponent(String name, String type, String owningTable, String sql, int foreignKeyFlag, int foreignKeyCount) {
            this.name = name;
            this.type = type;
            this.owningTable = owningTable;
            this.sql = sql;
            this.foreignKeyFlag = foreignKeyFlag;
            this.foreignKeyCount = foreignKeyCount;
        }

        public String getName() {
            return name;
        }

        public void setName(String name) {
            this.name = name;
        }

        public String getType() {
            return type;
        }

        public void setType(String type) {
            this.type = type;
        }

        public String getOwningTable() {
            return owningTable;
        }

        public void setOwningTable(String owningTable) {
            this.owningTable = owningTable;
        }

        public String getSql() {
            return sql;
        }

        public void setSql(String sql) {
            this.sql = sql;
        }

        public int getForeignKeyFlag() {
            return foreignKeyFlag;
        }

        public void setForeignKeyFlag(int foreignKeyFlag) {
            this.foreignKeyFlag = foreignKeyFlag;
        }

        public boolean isForeignKey() {
            return this.foreignKeyFlag > 0;
        }

        public int getForeignKeyCount() {
            return foreignKeyCount;
        }

        public void setForeignKeyCount(int foreignKeyCount) {
            this.foreignKeyCount = foreignKeyCount;
        }

        /**
         * Retrieve the number of rows returned by PRAGMA foreign_key_list
         * @param db    The SupportSQLiteDatabase to access
         * @return      The number of rows i.e. number of Foreign Key constraints
         */
        private int getForeignKeyCount(SupportSQLiteDatabase db) {
            int rv =0;
            Cursor csr = db.query("SELECT count(*) FROM pragma_foreign_key_list('" + this.name + "');");
            if (csr.moveToFirst()) {
                rv = csr.getInt(0);
            }
            csr.close();
            return rv;
        }
    }
}

Working Example

The Asset database

The asset database contains 3 tables, person, company and company_person_map

person DDL is

CREATE TABLE person (personid INTEGER PRIMARY KEY, firstName TEXT, lastName TEXT, middleNames TEXT, dateOfBirth DATE);

Room expects:-

CREATE TABLE IF NOT EXISTS `Person` (`personid` INTEGER, `firstName` TEXT, `lastName` TEXT, `middleNames` TEXT, `dateOfBirth` INTEGER, PRIMARY KEY(`personid`))
  • note the type of DATE v INTEGER for the dateOfBirth column which room would not accept so an expected … found ….

company DDL is

CREATE TABLE company (companyid INTEGER PRIMARY KEY, companyName TEXT, city TEXT, state TEXT, country TEXT, notes TEXT);

Room expects:-

CREATE TABLE IF NOT EXISTS `company` (`companyid` INTEGER, `companyName` TEXT, `city` TEXT, `state` TEXT, `country` TEXT, `notes` TEXT, PRIMARY KEY(`companyid`))
  • both are comparable room would accept

company_person_map DDL is

CREATE TABLE company_person_map (
companyid_map INTEGER NOT NULL REFERENCES company(companyid) ON DELETE CASCADE ON UPDATE CASCADE,
personid_map INTEGER NOT NULL REFERENCES person(personid) ON DELETE CASCADE ON UPDATE CASCADE, 
PRIMARY KEY (companyid_map, personid_map));

Room expects:-

CREATE TABLE IF NOT EXISTS `company_person_map` (
`companyid_map` INTEGER NOT NULL,`personid_map` INTEGER NOT NULL, 
PRIMARY KEY(`companyid_map`, `personid_map`), 
FOREIGN KEY(`companyid_map`) REFERENCES `company`(`companyid`) ON UPDATE CASCADE ON DELETE CASCADE , 
FOREIGN KEY(`personid_map`) REFERENCES `Person`(`personid`) ON UPDATE CASCADE ON DELETE CASCADE )
  • although they are significantly different, they are comparable, room would accept

The asset table contains the following data:-

person

enter image description here

company

enter image description here

company_person_map

enter image description here

  • person 11 is not mapped to a company.

AllDao is

  • none of the inserts are used, and not all the queries either.

The invoking Activity is :-

public class MainActivity extends AppCompatActivity {

    OtherDatabase dbOther;
    AllDao daoOther;

    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.activity_main);
        dbOther = OtherDatabase.getInstance(this);
        daoOther = dbOther.getAllDao();
        for(Person p: daoOther.getAllPeople()) {
            Log.d("DBINFO",
                    "Person is " + p.firstName
                            + " " + p.middleNames
                            + " " + p.lastName
                            + " Date of Birth is " + p.dateOfBirth
                            + " ID is " + p.id
            );
        }
        for(CompanyWithPeople cwp: dao.getCompanyWithPeople()) {
            logCompany(cwp.company,"DBINFO","");
            for(Person p: cwp.person) {
                logPerson(p,"DBINFO","\n\t");
            }
        }
    }

    void logCompany(Company c, String tag, String lineFeed) {
        Log.d(tag,lineFeed + "Company is " + c.companyName
                + " Location is " + c.city + ", " + c.state + ", " + c.country
                + " ID is " + c.companyId
                + "\n\t Notes: " + c.notes
        );
    }

    void logPerson(Person p, String tag, String lineFeed) {
        Log.d(tag, lineFeed + p.firstName + " " + p.middleNames
                        + " " + p.lastName + " Date of Birth is " + p.dateOfBirth
                        + " ID is " + p.id
        );
    }
}

Result

2021-11-28 19:56:01.928 D/DBINFO: Person is Robert John Smith Date of Birth is Sat Dec 27 17:46:33 GMT+10:00 1969 ID is 1
2021-11-28 19:56:01.929 D/DBINFO: Person is Julie Mary Armstrong Date of Birth is Mon Jan 12 23:22:04 GMT+10:00 1970 ID is 2
2021-11-28 19:56:01.929 D/DBINFO: Person is Andrea Susan Stewart Date of Birth is Mon Jan 05 04:56:09 GMT+10:00 1970 ID is 3
2021-11-28 19:56:01.929 D/DBINFO: Person is Mary Belinda Allway Date of Birth is Mon Jan 12 00:15:21 GMT+10:00 1970 ID is 4
2021-11-28 19:56:01.929 D/DBINFO: Person is Lisa Elizabeth Brooks Date of Birth is Sat Jan 03 03:51:21 GMT+10:00 1970 ID is 5
2021-11-28 19:56:01.930 D/DBINFO: Person is Stephen Colin Cobbs Date of Birth is Tue Jan 06 14:01:55 GMT+10:00 1970 ID is 6
2021-11-28 19:56:01.930 D/DBINFO: Person is Breane Cath Davidson Date of Birth is Thu Jan 01 22:30:14 GMT+10:00 1970 ID is 7
2021-11-28 19:56:01.930 D/DBINFO: Person is Trevor Arthur Frankston Date of Birth is Sat Jan 10 03:47:02 GMT+10:00 1970 ID is 8
2021-11-28 19:56:01.930 D/DBINFO: Person is George Howard Erksine Date of Birth is Sun Jan 11 00:47:02 GMT+10:00 1970 ID is 9
2021-11-28 19:56:01.930 D/DBINFO: Person is Uriah Stanley Jefferson Date of Birth is Mon Dec 29 19:11:31 GMT+10:00 1969 ID is 10
2021-11-28 19:56:01.931 D/DBINFO: Person is Valerie Alana Singleton Date of Birth is Thu Jan 01 11:45:07 GMT+10:00 1970 ID is 11
2021-11-28 19:56:01.931 D/DBINFO: Person is Vladimir Oscar Whitworth Date of Birth is Sat Jan 10 00:29:45 GMT+10:00 1970 ID is 12
2021-11-28 19:56:01.936 D/DBINFO: Company is Allbright Construction Location is Sydney, NSW, Australia ID is 1
         Notes: 
2021-11-28 19:56:01.936 D/DBINFO:   Julie Mary Armstrong Date of Birth is Mon Jan 12 23:22:04 GMT+10:00 1970 ID is 2
2021-11-28 19:56:01.936 D/DBINFO:   Mary Belinda Allway Date of Birth is Mon Jan 12 00:15:21 GMT+10:00 1970 ID is 4
2021-11-28 19:56:01.937 D/DBINFO:   Stephen Colin Cobbs Date of Birth is Tue Jan 06 14:01:55 GMT+10:00 1970 ID is 6
2021-11-28 19:56:01.937 D/DBINFO:   Trevor Arthur Frankston Date of Birth is Sat Jan 10 03:47:02 GMT+10:00 1970 ID is 8
2021-11-28 19:56:01.937 D/DBINFO:   Uriah Stanley Jefferson Date of Birth is Mon Dec 29 19:11:31 GMT+10:00 1969 ID is 10
2021-11-28 19:56:01.937 D/DBINFO:   Vladimir Oscar Whitworth Date of Birth is Sat Jan 10 00:29:45 GMT+10:00 1970 ID is 12
2021-11-28 19:56:01.937 D/DBINFO: Company is Dextronics Location is Slough, Berkshire, England ID is 2
         Notes: 
2021-11-28 19:56:01.937 D/DBINFO:   Robert John Smith Date of Birth is Sat Dec 27 17:46:33 GMT+10:00 1969 ID is 1
2021-11-28 19:56:01.938 D/DBINFO:   Andrea Susan Stewart Date of Birth is Mon Jan 05 04:56:09 GMT+10:00 1970 ID is 3
2021-11-28 19:56:01.938 D/DBINFO:   Lisa Elizabeth Brooks Date of Birth is Sat Jan 03 03:51:21 GMT+10:00 1970 ID is 5
2021-11-28 19:56:01.938 D/DBINFO:   Breane Cath Davidson Date of Birth is Thu Jan 01 22:30:14 GMT+10:00 1970 ID is 7
2021-11-28 19:56:01.938 D/DBINFO:   George Howard Erksine Date of Birth is Sun Jan 11 00:47:02 GMT+10:00 1970 ID is 9

Leave a Comment